diff mbox series

[v1] scsi: qla4xxx: Add length check when paring nlattrs

Message ID 20230723080053.3714534-1-linma@zju.edu.cn
State New
Headers
Series [v1] scsi: qla4xxx: Add length check when paring nlattrs |

Commit Message

Lin Ma July 23, 2023, 8 a.m. UTC 
  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(+)

Comments

Chris Leech July 25, 2023, 5:28 p.m. UTC | #1 
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>
Martin K. Petersen July 26, 2023, 1:56 a.m. UTC | #2 
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!
Martin K. Petersen July 31, 2023, 7:45 p.m. UTC | #3 
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 mbox series

Patch

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) {