Skip to content

Commit

Permalink
scsi: iscsi: Add strlen() check in iscsi_if_set{_host}_param()
Browse files Browse the repository at this point in the history
[ Upstream commit ce51c81 ]

The functions iscsi_if_set_param() and iscsi_if_set_host_param() convert an
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()), dirty data on the heap can
lead to an OOB access 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
fd7255f ("[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 a54a52c ("[SCSI] iscsi: fixup set/get param
functions"), the code treated the nlattr as C string while forgetting to
add any strlen checks(), opening the possibility of an OOB access.

Fix 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: fd7255f ("[SCSI] iscsi: add sysfs attrs for uspace sync up")
Fixes: 1d9bf13 ("[SCSI] iscsi class: add iscsi host set param event")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
Link: https://lore.kernel.org/r/20230723075820.3713119-1-linma@zju.edu.cn
Reviewed-by: Chris Leech <cleech@redhat.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
f0rm2l1n authored and gregkh committed Sep 13, 2023
1 parent bb8d101 commit 85b8c28
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions drivers/scsi/scsi_transport_iscsi.c
Expand Up @@ -3029,6 +3029,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);
Expand Down Expand Up @@ -3202,6 +3206,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);
Expand Down

0 comments on commit 85b8c28

Please sign in to comment.