Skip to content

Commit ce51c81

Browse files
f0rm2l1nmartinkpetersen
authored andcommitted
scsi: iscsi: Add strlen() check in iscsi_if_set{_host}_param()
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>
1 parent 971dfcb commit ce51c81

File tree

1 file changed

+8
-0
lines changed

1 file changed

+8
-0
lines changed

drivers/scsi/scsi_transport_iscsi.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3030,6 +3030,10 @@ iscsi_if_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev, u
30303030
if (!conn || !session)
30313031
return -EINVAL;
30323032

3033+
/* data will be regarded as NULL-ended string, do length check */
3034+
if (strlen(data) > ev->u.set_param.len)
3035+
return -EINVAL;
3036+
30333037
switch (ev->u.set_param.param) {
30343038
case ISCSI_PARAM_SESS_RECOVERY_TMO:
30353039
sscanf(data, "%d", &value);
@@ -3203,6 +3207,10 @@ iscsi_set_host_param(struct iscsi_transport *transport,
32033207
return -ENODEV;
32043208
}
32053209

3210+
/* see similar check in iscsi_if_set_param() */
3211+
if (strlen(data) > ev->u.set_host_param.len)
3212+
return -EINVAL;
3213+
32063214
err = transport->set_host_param(shost, ev->u.set_host_param.param,
32073215
data, ev->u.set_host_param.len);
32083216
scsi_host_put(shost);

0 commit comments

Comments
 (0)