[PATCH v1 2/2] scsi: iscsi: Add strlen check in iscsi_if_set_{host}_param

[PATCH v1 2/2] scsi: iscsi: Add strlen check in iscsi_if_set_{host}_param

[Lin Ma]

The function iscsi_if_set_param and iscsi_if_set_host_param converts
nlattr payload to type char* and then call C string handling functions
like sscanf and kstrdup.

  char *data = (char*)ev + sizeof(*ev);
  ...
  sscanf(data, "%d", &value);

However, since the nlattr is provided by the user-space program and
the nlmsg skb is allocated with GFP_KERNEL instead of GFP_ZERO flag
(see netlink_alloc_large_skb in netlink_sendmsg), the dirty data
remained in the heap can cause OOB read for those string handling
functions.

By investigating how the bug is introduced, we find it is really
interesting as the old version parsing code starting from commit
fd7255f51a13 ("[SCSI] iscsi: add sysfs attrs for uspace sync up")
treated the nlattr as integer bytes instead of string and had length
check in iscsi_copy_param.

  if (ev->u.set_param.len != sizeof(uint32_t))
    BUG();

But, since the commit a54a52caad4b ("[SCSI] iscsi: fixup set/get param
functions"), code treated the nlattr as C string while forggeting to add
any strlen checks, hence leave the possibility of OOB.

This patch fixes the potential OOB by adding the strlen check before
accessing the buf. If the data passes this check, all low-level
set_param handlers can safely treat this buf as legal C string.

Fixes: fd7255f51a13 ("[SCSI] iscsi: add sysfs attrs for uspace sync up")
Fixes: 1d9bf13a9cf9 ("[SCSI] iscsi class: add iscsi host set param event")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 drivers/scsi/scsi_transport_iscsi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 62b24f1c0232..8ade01da3045 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -3030,6 +3030,10 @@ iscsi_if_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev, u
 	if (!conn || !session)
 		return -EINVAL;
 
+	/* data will be regarded as NULL-ended string, do length check */
+	if (strlen(data) > ev->u.set_param.len)
+		return -EINVAL;
+
 	switch (ev->u.set_param.param) {
 	case ISCSI_PARAM_SESS_RECOVERY_TMO:
 		sscanf(data, "%d", &value);
@@ -3203,6 +3207,10 @@ iscsi_set_host_param(struct iscsi_transport *transport,
 		return -ENODEV;
 	}
 
+	/* see similar check in iscsi_if_set_param() */
+	if (strlen(data) > ev->u.set_host_param.len)
+		return -EINVAL;
+
 	err = transport->set_host_param(shost, ev->u.set_host_param.param,
 					data, ev->u.set_host_param.len);
 	scsi_host_put(shost);
-- 
2.17.1

Re: [PATCH v1 2/2] scsi: iscsi: Add strlen check in iscsi_if_set_{host}_param

[Martin K. Petersen]

Lin,

> The function iscsi_if_set_param and iscsi_if_set_host_param converts
> nlattr payload to type char* and then call C string handling functions
> like sscanf and kstrdup.

Applied to 6.6/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v1 2/2] scsi: iscsi: Add strlen check in iscsi_if_set_{host}_param

[Lin Ma]

The function iscsi_if_set_param and iscsi_if_set_host_param converts
nlattr payload to type char* and then call C string handling functions
like sscanf and kstrdup.

  char *data = (char*)ev + sizeof(*ev);
  ...
  sscanf(data, "%d", &value);

However, since the nlattr is provided by the user-space program and
the nlmsg skb is allocated with GFP_KERNEL instead of GFP_ZERO flag
(see netlink_alloc_large_skb in netlink_sendmsg), the dirty data
remained in the heap can cause OOB read for those string handling
functions.

By investigating how the bug is introduced, we find it is really
interesting as the old version parsing code starting from commit
fd7255f51a13 ("[SCSI] iscsi: add sysfs attrs for uspace sync up")
treated the nlattr as integer bytes instead of string and had length
check in iscsi_copy_param.

  if (ev->u.set_param.len != sizeof(uint32_t))
    BUG();

But, since the commit a54a52caad4b ("[SCSI] iscsi: fixup set/get param
functions"), code treated the nlattr as C string while forggeting to add
any strlen checks, hence leave the possibility of OOB.

This patch fixes the potential OOB by adding the strlen check before
accessing the buf. If the data passes this check, all low-level
set_param handlers can safely treat this buf as legal C string.

Fixes: fd7255f51a13 ("[SCSI] iscsi: add sysfs attrs for uspace sync up")
Fixes: 1d9bf13a9cf9 ("[SCSI] iscsi class: add iscsi host set param event")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
V1 -> V2: resend with correct CC list

 drivers/scsi/scsi_transport_iscsi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 62b24f1c0232..8ade01da3045 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -3030,6 +3030,10 @@ iscsi_if_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev, u
 	if (!conn || !session)
 		return -EINVAL;
 
+	/* data will be regarded as NULL-ended string, do length check */
+	if (strlen(data) > ev->u.set_param.len)
+		return -EINVAL;
+
 	switch (ev->u.set_param.param) {
 	case ISCSI_PARAM_SESS_RECOVERY_TMO:
 		sscanf(data, "%d", &value);
@@ -3203,6 +3207,10 @@ iscsi_set_host_param(struct iscsi_transport *transport,
 		return -ENODEV;
 	}
 
+	/* see similar check in iscsi_if_set_param() */
+	if (strlen(data) > ev->u.set_host_param.len)
+		return -EINVAL;
+
 	err = transport->set_host_param(shost, ev->u.set_host_param.param,
 					data, ev->u.set_host_param.len);
 	scsi_host_put(shost);
-- 
2.17.1

Re: [PATCH v1 2/2] scsi: iscsi: Add strlen check in iscsi_if_set_{host}_param

[Chris Leech]

On Tue, Jul 25, 2023 at 10:45:45AM +0800, Lin Ma wrote:
> The function iscsi_if_set_param and iscsi_if_set_host_param converts
> nlattr payload to type char* and then call C string handling functions
> like sscanf and kstrdup.
> 
>   char *data = (char*)ev + sizeof(*ev);
>   ...
>   sscanf(data, "%d", &value);
> 
> However, since the nlattr is provided by the user-space program and
> the nlmsg skb is allocated with GFP_KERNEL instead of GFP_ZERO flag
> (see netlink_alloc_large_skb in netlink_sendmsg), the dirty data
> remained in the heap can cause OOB read for those string handling
> functions.

Reviewed-by: Chris Leech <cleech@redhat.com>