| Message ID | 20230725024545.428519-1-linma@zju.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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>
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);
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(+)