qla4xxx: overflow in qla4xxx_set_chap_entry()

PATCH In review Dan Carpenter 2 participants 5 messages Nov 13, 2013 – Nov 14, 2013

Subsystems (3)

SCSI Kernel janitors QLogic QLA4XXX iSCSI driver

lore

Share & tools

lore

Dan Carpenter

author

2013-11-13 07:48 UTC #0 lore raw

PATCH 1/1 qla4xxx: overflow in qla4xxx_set_chap_entry()

We should cap the size of memcpy() because it comes from the network and can't be trusted.

Fixes 26ffd7b45fe9 ('[SCSI] qla4xxx: Add support to set CHAP entries')

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/qla4xxx/ql4_os.c +7 -4

index a28d5e6..cf174a4 100644
@@ -802,6 +802,7 @@ static int qla4xxx_set_chap_entry(struct Scsi_Host *shost, void *data, int len)
 	int type;
 	int rem = len;
 	int rc = 0;
+	int size;
 
 	memset(&chap_rec, 0, sizeof(chap_rec));

@@ -816,12 +817,14 @@ static int qla4xxx_set_chap_entry(struct Scsi_Host *shost, void *data, int len)
 			chap_rec.chap_type = param_info->value[0];
 			break;
 		case ISCSI_CHAP_PARAM_USERNAME:
-			memcpy(chap_rec.username, param_info->value,
-			       param_info->len);
+			size = min_t(size_t, sizeof(chap_rec.username),
+				     param_info->len);
+			memcpy(chap_rec.username, param_info->value, size);
 			break;
 		case ISCSI_CHAP_PARAM_PASSWORD:
-			memcpy(chap_rec.password, param_info->value,
-			       param_info->len);
+			size = min_t(size_t, sizeof(chap_rec.password),
+				     param_info->len);
+			memcpy(chap_rec.password, param_info->value, size);
 			break;
 		case ISCSI_CHAP_PARAM_PASSWORD_LEN:
 			chap_rec.password_length = param_info->value[0];

-----Original Message-----
From: Dan Carpenter <[email protected]>
Date: Wednesday, 13 November 2013 1:18 pm
To: Vikas <[email protected]>, Adheer Chandravanshi <[email protected]>
Cc: Dept-Eng iSCSI Driver <[email protected]>, "James E.J. Bottomley" <[email protected]>, scsi <[email protected]>, "[email protected]" <[email protected]>
Subject: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

Quoted 2 lines

We should cap the size of memcpy() because it comes from the network and can't be trusted.

This patch is on assumption that data is coming from network, but in this case data come from application (iscsiadm) with correct length.

Quoted 37 lines

Fixes 26ffd7b45fe9 ('[SCSI] qla4xxx: Add support to set CHAP entries')

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/qla4xxx/ql4_os.c +7 -4

index a28d5e6..cf174a4 100644
@@ -802,6 +802,7 @@ static int qla4xxx_set_chap_entry(struct Scsi_Host
 *shost, void *data, int len)
 	int type;
 	int rem = len;
 	int rc = 0;
+	int size;
 
 	memset(&chap_rec, 0, sizeof(chap_rec));

@@ -816,12 +817,14 @@ static int qla4xxx_set_chap_entry(struct Scsi_Host
 *shost, void *data, int len)
 			chap_rec.chap_type = param_info->value[0];
 			break;
 		case ISCSI_CHAP_PARAM_USERNAME:
-			memcpy(chap_rec.username, param_info->value,
-			       param_info->len);
+			size = min_t(size_t, sizeof(chap_rec.username),
+				     param_info->len);
+			memcpy(chap_rec.username, param_info->value, size);
 			break;
 		case ISCSI_CHAP_PARAM_PASSWORD:
-			memcpy(chap_rec.password, param_info->value,
-			       param_info->len);
+			size = min_t(size_t, sizeof(chap_rec.password),
+				     param_info->len);
+			memcpy(chap_rec.password, param_info->value, size);
 			break;
 		case ISCSI_CHAP_PARAM_PASSWORD_LEN:

			chap_rec.password_length = param_info->value[0];

Dan Carpenter

author

2013-11-13 12:08 UTC #2 lore raw

Quoted 12 lines · Vikas Chaudhary

-----Original Message-----
From: Dan Carpenter <[email protected]>
Date: Wednesday, 13 November 2013 1:18 pm
To: Vikas <[email protected]>, Adheer Chandravanshi <[email protected]>
Cc: Dept-Eng iSCSI Driver <[email protected]>, "James E.J. Bottomley" <[email protected]>, scsi <[email protected]>, "[email protected]" <[email protected]>
Subject: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

Quoted 6 lines

Quoted 2 lines
We should cap the size of memcpy() because it comes from the network and can't be trusted.
This patch is on assumption that data is coming from network, but in this case data come from application (iscsiadm) with correct length.

No, that doesn't work. We don't trust user space.

regards,
dan carpenter

Dan Carpenter

author

2013-11-13 14:06 UTC #3 lore raw

Quoted 13 lines · Dan Carpenter

Quoted 12 lines · Vikas Chaudhary
-----Original Message-----
From: Dan Carpenter <[email protected]>
Date: Wednesday, 13 November 2013 1:18 pm
To: Vikas <[email protected]>, Adheer Chandravanshi <[email protected]>
Cc: Dept-Eng iSCSI Driver <[email protected]>, "James E.J. Bottomley" <[email protected]>, scsi <[email protected]>, "[email protected]" <[email protected]>
Subject: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
We should cap the size of memcpy() because it comes from the network and can't be trusted.
This patch is on assumption that data is coming from network, but in this case data come from application (iscsiadm) with correct length.
No, that doesn't work. We don't trust user space.

Btw, the is especially true with network namespaces... These days
anyone who is ns_capable() could overflow the buffer after:
df008c91f835 ('net: Allow userns root to control llc, netfilter, netlink, packet, and xfrm')

regards,
dan carpenter

Vikas Chaudhary

Acked-by

2013-11-14 03:53 UTC #4 lore raw

-----Original Message-----
From: Dan Carpenter <[email protected]>
Date: Wednesday, 13 November 2013 7:36 pm
To: Vikas <[email protected]>
Cc: Adheer Chandravanshi <[email protected]>, Dept-Eng iSCSI Driver <[email protected]>, "James E.J. Bottomley" <[email protected]>, scsi <[email protected]>, "[email protected]" <[email protected]>, "Eric W. Biederman" <[email protected]>
Subject: Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

Quoted 23 lines · Dan Carpenter

Quoted 22 lines · Vikas Chaudhary
Quoted 9 lines
-----Original Message-----
From: Dan Carpenter <[email protected]>
Date: Wednesday, 13 November 2013 1:18 pm
To: Vikas <[email protected]>, Adheer Chandravanshi <[email protected]>
Cc: Dept-Eng iSCSI Driver <[email protected]>, "James E.J. Bottomley" <[email protected]>, scsi
<[email protected]>,
Quoted 8 lines
"[email protected]" <[email protected]>
Subject: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()
Quoted 2 lines
We should cap the size of memcpy() because it comes from the network and can't be trusted.
This patch is on assumption that data is coming from network, but in this case data come from application (iscsiadm) with correct
length.
Quoted 1 line

Quoted 6 lines

Quoted 1 line
No, that doesn't work. We don't trust user space.
Btw, the is especially true with network namespaces... These days anyone who is ns_capable() could overflow the buffer after:
df008c91f835 ('net: Allow userns root to control llc, netfilter, netlink, packet, and xfrm')

Agreed, We can¹t trust user space.

Acked-by Vikas Chaudhary <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html