[patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

[patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

[Dan Carpenter]

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 <dan.carpenter@oracle.com>

diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index a28d5e6..cf174a4 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -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];

Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

[Vikas Chaudhary]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]



-----Original Message-----
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wednesday, 13 November 2013 1:18 pm
To: Vikas <vikas.chaudhary@qlogic.com>, Adheer Chandravanshi
<adheer.chandravanshi@qlogic.com>
Cc: Dept-Eng iSCSI Driver <Dept-iSCSIDriver@qlogic.com>, "James E.J.
Bottomley" <JBottomley@parallels.com>, scsi <linux-scsi@vger.kernel.org>,
"kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>
Subject: [patch] [SCSI] 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.

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


>
>Fixes: 26ffd7b45fe9 ('[SCSI] qla4xxx: Add support to set CHAP entries')
>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
>diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
>index a28d5e6..cf174a4 100644
>--- a/drivers/scsi/qla4xxx/ql4_os.c
>+++ b/drivers/scsi/qla4xxx/ql4_os.c
>@@ -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];


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4665 bytes --]

Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

[Dan Carpenter]

On Wed, Nov 13, 2013 at 11:52:37AM +0000, Vikas Chaudhary wrote:
> 
> 
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Wednesday, 13 November 2013 1:18 pm
> To: Vikas <vikas.chaudhary@qlogic.com>, Adheer Chandravanshi
> <adheer.chandravanshi@qlogic.com>
> Cc: Dept-Eng iSCSI Driver <Dept-iSCSIDriver@qlogic.com>, "James E.J.
> Bottomley" <JBottomley@parallels.com>, scsi <linux-scsi@vger.kernel.org>,
> "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>
> Subject: [patch] [SCSI] 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.
> 
> 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

Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

[Dan Carpenter]

On Wed, Nov 13, 2013 at 03:08:12PM +0300, Dan Carpenter wrote:
> On Wed, Nov 13, 2013 at 11:52:37AM +0000, Vikas Chaudhary wrote:
> > 
> > 
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Date: Wednesday, 13 November 2013 1:18 pm
> > To: Vikas <vikas.chaudhary@qlogic.com>, Adheer Chandravanshi
> > <adheer.chandravanshi@qlogic.com>
> > Cc: Dept-Eng iSCSI Driver <Dept-iSCSIDriver@qlogic.com>, "James E.J.
> > Bottomley" <JBottomley@parallels.com>, scsi <linux-scsi@vger.kernel.org>,
> > "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>
> > Subject: [patch] [SCSI] 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.
> > 
> > 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

Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

[Vikas Chaudhary]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]



-----Original Message-----
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wednesday, 13 November 2013 7:36 pm
To: Vikas <vikas.chaudhary@qlogic.com>
Cc: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>, Dept-Eng iSCSI
Driver <Dept-iSCSIDriver@qlogic.com>, "James E.J. Bottomley"
<JBottomley@parallels.com>, scsi <linux-scsi@vger.kernel.org>,
"kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>, "Eric
W. Biederman" <ebiederm@xmission.com>
Subject: Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()

>On Wed, Nov 13, 2013 at 03:08:12PM +0300, Dan Carpenter wrote:
>> On Wed, Nov 13, 2013 at 11:52:37AM +0000, Vikas Chaudhary wrote:
>> > 
>> > 
>> > -----Original Message-----
>> > From: Dan Carpenter <dan.carpenter@oracle.com>
>> > Date: Wednesday, 13 November 2013 1:18 pm
>> > To: Vikas <vikas.chaudhary@qlogic.com>, Adheer Chandravanshi
>> > <adheer.chandravanshi@qlogic.com>
>> > Cc: Dept-Eng iSCSI Driver <Dept-iSCSIDriver@qlogic.com>, "James E.J.
>> > Bottomley" <JBottomley@parallels.com>, scsi
>><linux-scsi@vger.kernel.org>,
>> > "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>
>> > Subject: [patch] [SCSI] 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.
>> > 
>> > 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')

Agreed, We can¹t trust user space.

Acked-by: Vikas Chaudhary <vikas.chaudhary@qlogic.com>




[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4541 bytes --]