| Message ID | 20230723080053.3714534-1-linma@zju.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] scsi: qla4xxx: Add length check when paring nlattrs | expand |
On Sun, Jul 23, 2023 at 04:00:53PM +0800, Lin Ma wrote: > There are three places that qla4xxx looply parses nlattrs > * qla4xxx_set_chap_entry(...) > * qla4xxx_iface_set_param(...) > * qla4xxx_sysfs_ddb_set_param(...) > and each of them directly converts the nlattr to specific pointer of > structure without length checking. This could be dangerous as those > attributes are not validated before and a malformed nlattr (e.g., length > 0) could result in an OOB read that leaks heap dirty data. > > This patch adds the nla_len check before accessing the nlattr data and > error return EINVAL if the length check fails. Reviewed-by: Chris Leech <cleech@redhat.com>
Lin, > There are three places that qla4xxx looply parses nlattrs > * qla4xxx_set_chap_entry(...) > * qla4xxx_iface_set_param(...) > * qla4xxx_sysfs_ddb_set_param(...) Applied to 6.6/scsi-staging, thanks!
On Sun, 23 Jul 2023 16:00:53 +0800, Lin Ma wrote: > There are three places that qla4xxx looply parses nlattrs > * qla4xxx_set_chap_entry(...) > * qla4xxx_iface_set_param(...) > * qla4xxx_sysfs_ddb_set_param(...) > and each of them directly converts the nlattr to specific pointer of > structure without length checking. This could be dangerous as those > attributes are not validated before and a malformed nlattr (e.g., length > 0) could result in an OOB read that leaks heap dirty data. > > [...] Applied to 6.6/scsi-queue, thanks! [1/1] scsi: qla4xxx: Add length check when paring nlattrs https://git.kernel.org/mkp/scsi/c/47cd3770e31d
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c index b2a3988e1e15..675332e49a7b 100644 --- a/drivers/scsi/qla4xxx/ql4_os.c +++ b/drivers/scsi/qla4xxx/ql4_os.c @@ -968,6 +968,11 @@ static int qla4xxx_set_chap_entry(struct Scsi_Host *shost, void *data, int len) memset(&chap_rec, 0, sizeof(chap_rec)); nla_for_each_attr(attr, data, len, rem) { + if (nla_len(attr) < sizeof(*param_info)) { + rc = -EINVAL; + goto exit_set_chap; + } + param_info = nla_data(attr); switch (param_info->param) { @@ -2750,6 +2755,11 @@ qla4xxx_iface_set_param(struct Scsi_Host *shost, void *data, uint32_t len) } nla_for_each_attr(attr, data, len, rem) { + if (nla_len(attr) < sizeof(*iface_param)) { + rval = -EINVAL; + goto exit_init_fw_cb; + } + iface_param = nla_data(attr); if (iface_param->param_type == ISCSI_NET_PARAM) { @@ -8104,6 +8114,11 @@ qla4xxx_sysfs_ddb_set_param(struct iscsi_bus_flash_session *fnode_sess, memset((void *)&chap_tbl, 0, sizeof(chap_tbl)); nla_for_each_attr(attr, data, len, rem) { + if (nla_len(attr) < sizeof(*fnode_param)) { + rc = -EINVAL; + goto exit_set_param; + } + fnode_param = nla_data(attr); switch (fnode_param->param) {
There are three places that qla4xxx looply parses nlattrs * qla4xxx_set_chap_entry(...) * qla4xxx_iface_set_param(...) * qla4xxx_sysfs_ddb_set_param(...) and each of them directly converts the nlattr to specific pointer of structure without length checking. This could be dangerous as those attributes are not validated before and a malformed nlattr (e.g., length 0) could result in an OOB read that leaks heap dirty data. This patch adds the nla_len check before accessing the nlattr data and error return EINVAL if the length check fails. Fixes: 26ffd7b45fe9 ("[SCSI] qla4xxx: Add support to set CHAP entries") Fixes: 1e9e2be3ee03 ("[SCSI] qla4xxx: Add flash node mgmt support") Fixes: 00c31889f751 ("[SCSI] qla4xxx: fix data alignment and use nl helpers") Signed-off-by: Lin Ma <linma@zju.edu.cn> --- drivers/scsi/qla4xxx/ql4_os.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)