* [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()
@ 2013-11-13 7:48 Dan Carpenter
2013-11-13 11:52 ` Vikas Chaudhary
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2013-11-13 7:48 UTC (permalink / raw)
To: Vikas Chaudhary, Adheer Chandravanshi
Cc: iscsi-driver, James E.J. Bottomley, linux-scsi, kernel-janitors
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];
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()
2013-11-13 7:48 [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry() Dan Carpenter
@ 2013-11-13 11:52 ` Vikas Chaudhary
2013-11-13 12:08 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Vikas Chaudhary @ 2013-11-13 11:52 UTC (permalink / raw)
To: Dan Carpenter, Adheer Chandravanshi
Cc: Dept-Eng iSCSI Driver, James E.J. Bottomley, linux-scsi,
kernel-janitors@vger.kernel.org
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()
2013-11-13 11:52 ` Vikas Chaudhary
@ 2013-11-13 12:08 ` Dan Carpenter
2013-11-13 14:06 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2013-11-13 12:08 UTC (permalink / raw)
To: Vikas Chaudhary
Cc: Adheer Chandravanshi, Dept-Eng iSCSI Driver, James E.J. Bottomley,
linux-scsi, kernel-janitors@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()
2013-11-13 12:08 ` Dan Carpenter
@ 2013-11-13 14:06 ` Dan Carpenter
2013-11-14 3:53 ` Vikas Chaudhary
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2013-11-13 14:06 UTC (permalink / raw)
To: Vikas Chaudhary
Cc: Adheer Chandravanshi, Dept-Eng iSCSI Driver, James E.J. Bottomley,
linux-scsi, kernel-janitors@vger.kernel.org, Eric W. Biederman
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry()
2013-11-13 14:06 ` Dan Carpenter
@ 2013-11-14 3:53 ` Vikas Chaudhary
0 siblings, 0 replies; 5+ messages in thread
From: Vikas Chaudhary @ 2013-11-14 3:53 UTC (permalink / raw)
To: Dan Carpenter
Cc: Adheer Chandravanshi, Dept-Eng iSCSI Driver, James E.J. Bottomley,
linux-scsi, kernel-janitors@vger.kernel.org, Eric W. Biederman
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-14 3:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-13 7:48 [patch] [SCSI] qla4xxx: overflow in qla4xxx_set_chap_entry() Dan Carpenter
2013-11-13 11:52 ` Vikas Chaudhary
2013-11-13 12:08 ` Dan Carpenter
2013-11-13 14:06 ` Dan Carpenter
2013-11-14 3:53 ` Vikas Chaudhary
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox