[patch] [SCSI] aacraid: prevent ZERO_SIZE_PTR dereference

[patch] [SCSI] aacraid: prevent ZERO_SIZE_PTR dereference

[Dan Carpenter]

If "fibsize" is zero then it leads to a ZERO_SIZE_PTR dereference when
we dereference user_srbcmd.

Due to a missing capable() check in the compat ioctls then this error
can be triggered without CAP_SYS_RAWIO.  I have fixed that in a separate
patch.

Reported-by: Nico Golde <nico@ngolde.de>
Reported-by: Fabian Yamaguchi <fabs@goesec.de>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index d85ac1a..efd0ba3 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -511,7 +511,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		goto cleanup;
 	}
 
-	if (fibsize > (dev->max_fib_size - sizeof(struct aac_fibhdr))) {
+	if (fibsize == 0 ||
+	    fibsize > (dev->max_fib_size - sizeof(struct aac_fibhdr))) {
 		rcode = -EINVAL;
 		goto cleanup;
 	}

Re: [patch] [SCSI] aacraid: prevent ZERO_SIZE_PTR dereference

[James Bottomley]

On Tue, 2013-10-29 at 22:10 +0300, Dan Carpenter wrote:
> If "fibsize" is zero then it leads to a ZERO_SIZE_PTR dereference when
> we dereference user_srbcmd.
> 
> Due to a missing capable() check in the compat ioctls then this error
> can be triggered without CAP_SYS_RAWIO.  I have fixed that in a separate
> patch.
> 
> Reported-by: Nico Golde <nico@ngolde.de>
> Reported-by: Fabian Yamaguchi <fabs@goesec.de>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
> index d85ac1a..efd0ba3 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -511,7 +511,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
>  		goto cleanup;
>  	}
>  
> -	if (fibsize > (dev->max_fib_size - sizeof(struct aac_fibhdr))) {
> +	if (fibsize == 0 ||
> +	    fibsize > (dev->max_fib_size - sizeof(struct aac_fibhdr))) {

Not really if you want to catch actual undersize errors.  We read out of
the allocated data structure after this, so it had better be at least
sizeof(*user_srbcmd) rather than just not zero to make sure we aren't
reading off the end of the allocated size.

James

Re: [patch] [SCSI] aacraid: prevent ZERO_SIZE_PTR dereference

[Linus Torvalds]

On Tue, Oct 29, 2013 at 12:10 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> If "fibsize" is zero then it leads to a ZERO_SIZE_PTR dereference when
> we dereference user_srbcmd.

Btw, these "ZERO_SIZE_PTR dereference" issues aren't about
ZERO_SIZE_PTR, they are about overrunning the allocations. The
ZERO_SIZE_PTR pointer is a perfectly valid pointer and can be
dereferenced just fine, as long as you stay within the allocation
size.

Think about it.

So I really get the feeling that checking for a zero size is very
wrong. If we can access the ZERO_SIZE_PTR, that means that we at some
point don't check the size limits, and if that's true for the zero
size, I don't see why that wouldn't be true for *other* sizes too..

I didn't check this particular case, and maybe zero really is special.
But it's not clear at all why it should be. Can you explain why zero
is special here, and why the buffer overrun cannot happen with size 1,
for example?

Because quite frankly, from what I can tell, testing against zero is
absolutely *WRONG*. Any size less than the size of "struct
user_aac_srb" would seem to be bogus.

This has nothing to do with zero, or with ZERO_SIZE_PTR.

Tell me why I'm wrong.

                Linus

Re: [patch] [SCSI] aacraid: prevent ZERO_SIZE_PTR dereference

[Dan Carpenter]

You and James are right.  It should be checking against the sizeof().
I will send a v2 tomorrow.  Sorry about that.

regards,
dan carpenter

Re: [patch] [SCSI] aacraid: prevent ZERO_SIZE_PTR dereference

[Linus Torvalds]

On Tue, Oct 29, 2013 at 1:06 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> You and James are right.  It should be checking against the sizeof().
> I will send a v2 tomorrow.  Sorry about that.

Looking some more at this, I have to say that I absolutely detest
those aacraid structures. And I'm not sure that sizeof() is
necessarily the right thing for the minimum size.

The "struct user_aac_srb" includes a

        struct  user_sgmap      sg;

which has a count in it. But the actual "struct  user_sgmap" structure
is defined with a

        struct user_sgentry     sg[1];

in it, so the sizeof() of that structure basically gives the size of
an entry that has _one_ sgentry.

And it's not entirely clear that you absolutely have to have a minimum
of one sgentry. So I could imagine that there would be a zero-entry
case that doesn't have any scatter-gather entries at all (ie just the
status parts). So the "sizeof()" might actually end up giving a
minimum size that is too large *if* it is possible to not have those
scatter-gather entries at all?

Hmm? Somebody who knows this code, please speak up..

                Linus

Re: [patch] [SCSI] aacraid: prevent ZERO_SIZE_PTR dereference

[Dan Carpenter]

On Tue, Oct 29, 2013 at 10:10:07PM +0300, Dan Carpenter wrote:

> Due to a missing capable() check in the compat ioctls then this error
> can be triggered without CAP_SYS_RAWIO.  I have fixed that in a separate
> patch.

Actually, CAP_SYS_RAWIO is checked at the start of the function.

However my other patch which adds the check in the compat ioctl should
probably still be applied so it matches the regular ioctl.

regards,
dan carpenter

[patch] [SCSI] aacraid: missing capable() check in compat ioctl

[Dan Carpenter]

In d496f94d22d1 ('[SCSI] aacraid: fix security weakness') we added a
check on CAP_SYS_RAWIO to the ioctl.  The compat ioctls need the check
as well.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 408a42e..f0d432c 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -771,6 +771,8 @@ static long aac_compat_do_ioctl(struct aac_dev *dev, unsigned cmd, unsigned long
 static int aac_compat_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
 {
 	struct aac_dev *dev = (struct aac_dev *)sdev->host->hostdata;
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
 	return aac_compat_do_ioctl(dev, cmd, (unsigned long)arg);
 }
 

[patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()

[Dan Carpenter]

pthru32->dataxferlen comes from the user so we need to check that it's
not too large so we don't overflow the buffer.

Reported-by: Nico Golde <nico@ngolde.de>
Reported-by: Fabian Yamaguchi <fabs@goesec.de>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Please review this carefully because I have not tested it.

diff --git a/drivers/scsi/megaraid/megaraid_mm.c b/drivers/scsi/megaraid/megaraid_mm.c
index dfffd0f..a706927 100644
--- a/drivers/scsi/megaraid/megaraid_mm.c
+++ b/drivers/scsi/megaraid/megaraid_mm.c
@@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd, mraid_mmadp_t *adp, uioc_t *kioc)
 
 	pthru32->dataxferaddr	= kioc->buf_paddr;
 	if (kioc->data_dir & UIOC_WR) {
+		if (pthru32->dataxferlen > kioc->xferlen)
+			return -EINVAL;
 		if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
 						pthru32->dataxferlen)) {
 			return (-EFAULT);

Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()

[Kees Cook]

Hi,

On Wed, Oct 30, 2013 at 10:13 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> pthru32->dataxferlen comes from the user so we need to check that it's
> not too large so we don't overflow the buffer.
>
> Reported-by: Nico Golde <nico@ngolde.de>
> Reported-by: Fabian Yamaguchi <fabs@goesec.de>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

I don't see this in Linus's tree nor linux-next yet. Can someone pick this up?

Thanks!

-Kees

> ---
> Please review this carefully because I have not tested it.
>
> diff --git a/drivers/scsi/megaraid/megaraid_mm.c b/drivers/scsi/megaraid/megaraid_mm.c
> index dfffd0f..a706927 100644
> --- a/drivers/scsi/megaraid/megaraid_mm.c
> +++ b/drivers/scsi/megaraid/megaraid_mm.c
> @@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd, mraid_mmadp_t *adp, uioc_t *kioc)
>
>         pthru32->dataxferaddr   = kioc->buf_paddr;
>         if (kioc->data_dir & UIOC_WR) {
> +               if (pthru32->dataxferlen > kioc->xferlen)
> +                       return -EINVAL;
>                 if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
>                                                 pthru32->dataxferlen)) {
>                         return (-EFAULT);
>



-- 
Kees Cook
Chrome OS Security

RE: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()

[Saxena, Sumit]

>-----Original Message-----
>From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>Sent: Wednesday, October 30, 2013 10:44 PM
>To: DL-MegaRAID Linux
>Cc: James E.J. Bottomley; linux-scsi@vger.kernel.org; security@kernel.org;
>Nico Golde; Fabian Yamaguchi
>Subject: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
>
>pthru32->dataxferlen comes from the user so we need to check that it's
>not too large so we don't overflow the buffer.
>
>Reported-by: Nico Golde <nico@ngolde.de>
>Reported-by: Fabian Yamaguchi <fabs@goesec.de>
>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>---
>Please review this carefully because I have not tested it.
>
>diff --git a/drivers/scsi/megaraid/megaraid_mm.c
>b/drivers/scsi/megaraid/megaraid_mm.c
>index dfffd0f..a706927 100644
>--- a/drivers/scsi/megaraid/megaraid_mm.c
>+++ b/drivers/scsi/megaraid/megaraid_mm.c
>@@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd,
>mraid_mmadp_t *adp, uioc_t *kioc)
>
> 	pthru32->dataxferaddr	= kioc->buf_paddr;
> 	if (kioc->data_dir & UIOC_WR) {
>+		if (pthru32->dataxferlen > kioc->xferlen)
>+			return -EINVAL;
> 		if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
> 						pthru32->dataxferlen)) {
> 			return (-EFAULT);

Acked-by: Sumit Saxena <sumit.saxena@lsi.com>

Sumit

Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()

[Kees Cook]

On Wed, Jan 8, 2014 at 4:27 AM, Saxena, Sumit <Sumit.Saxena@lsi.com> wrote:
>
>
>>-----Original Message-----
>>From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>>Sent: Wednesday, October 30, 2013 10:44 PM
>>To: DL-MegaRAID Linux
>>Cc: James E.J. Bottomley; linux-scsi@vger.kernel.org; security@kernel.org;
>>Nico Golde; Fabian Yamaguchi
>>Subject: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
>>
>>pthru32->dataxferlen comes from the user so we need to check that it's
>>not too large so we don't overflow the buffer.
>>
>>Reported-by: Nico Golde <nico@ngolde.de>
>>Reported-by: Fabian Yamaguchi <fabs@goesec.de>
>>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>---
>>Please review this carefully because I have not tested it.
>>
>>diff --git a/drivers/scsi/megaraid/megaraid_mm.c
>>b/drivers/scsi/megaraid/megaraid_mm.c
>>index dfffd0f..a706927 100644
>>--- a/drivers/scsi/megaraid/megaraid_mm.c
>>+++ b/drivers/scsi/megaraid/megaraid_mm.c
>>@@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd,
>>mraid_mmadp_t *adp, uioc_t *kioc)
>>
>>       pthru32->dataxferaddr   = kioc->buf_paddr;
>>       if (kioc->data_dir & UIOC_WR) {
>>+              if (pthru32->dataxferlen > kioc->xferlen)
>>+                      return -EINVAL;
>>               if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
>>                                               pthru32->dataxferlen)) {
>>                       return (-EFAULT);
>
> Acked-by: Sumit Saxena <sumit.saxena@lsi.com>
>
> Sumit
>

Thanks for the Ack. Who normally picks patches for this area?

-Kees

-- 
Kees Cook
Chrome OS Security

RE: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()

[Saxena, Sumit]

>-----Original Message-----
>From: Kees Cook [mailto:keescook@google.com]
>Sent: Friday, January 10, 2014 12:05 AM
>To: Saxena, Sumit
>Cc: Dan Carpenter; DL-MegaRAID Linux; James E.J. Bottomley; linux-
>scsi@vger.kernel.org; security@kernel.org; Nico Golde; Fabian Yamaguchi
>Subject: Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
>
>On Wed, Jan 8, 2014 at 4:27 AM, Saxena, Sumit <Sumit.Saxena@lsi.com>
>wrote:
>>
>>
>>>-----Original Message-----
>>>From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>>>Sent: Wednesday, October 30, 2013 10:44 PM
>>>To: DL-MegaRAID Linux
>>>Cc: James E.J. Bottomley; linux-scsi@vger.kernel.org;
>>>security@kernel.org; Nico Golde; Fabian Yamaguchi
>>>Subject: [patch] [SCSI] megaraid: missing bounds check in
>>>mimd_to_kioc()
>>>
>>>pthru32->dataxferlen comes from the user so we need to check that it's
>>>not too large so we don't overflow the buffer.
>>>
>>>Reported-by: Nico Golde <nico@ngolde.de>
>>>Reported-by: Fabian Yamaguchi <fabs@goesec.de>
>>>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>---
>>>Please review this carefully because I have not tested it.
>>>
>>>diff --git a/drivers/scsi/megaraid/megaraid_mm.c
>>>b/drivers/scsi/megaraid/megaraid_mm.c
>>>index dfffd0f..a706927 100644
>>>--- a/drivers/scsi/megaraid/megaraid_mm.c
>>>+++ b/drivers/scsi/megaraid/megaraid_mm.c
>>>@@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd,
>mraid_mmadp_t
>>>*adp, uioc_t *kioc)
>>>
>>>       pthru32->dataxferaddr   = kioc->buf_paddr;
>>>       if (kioc->data_dir & UIOC_WR) {
>>>+              if (pthru32->dataxferlen > kioc->xferlen)
>>>+                      return -EINVAL;
>>>               if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
>>>                                               pthru32->dataxferlen)) {
>>>                       return (-EFAULT);
>>
>> Acked-by: Sumit Saxena <sumit.saxena@lsi.com>
>>
>> Sumit
>>
>
>Thanks for the Ack. Who normally picks patches for this area?
>
>-Kees
>
James Bottomley(Linux SCSI subsystem maintainer) should pick this patch.

Sumit
>--
>Kees Cook
>Chrome OS Security

Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()

[Kees Cook]

On Thu, Jan 9, 2014 at 11:53 AM, Saxena, Sumit <Sumit.Saxena@lsi.com> wrote:
>
>
>>-----Original Message-----
>>From: Kees Cook [mailto:keescook@google.com]
>>Sent: Friday, January 10, 2014 12:05 AM
>>To: Saxena, Sumit
>>Cc: Dan Carpenter; DL-MegaRAID Linux; James E.J. Bottomley; linux-
>>scsi@vger.kernel.org; security@kernel.org; Nico Golde; Fabian Yamaguchi
>>Subject: Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
>>
>>On Wed, Jan 8, 2014 at 4:27 AM, Saxena, Sumit <Sumit.Saxena@lsi.com>
>>wrote:
>>>
>>>
>>>>-----Original Message-----
>>>>From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>>>>Sent: Wednesday, October 30, 2013 10:44 PM
>>>>To: DL-MegaRAID Linux
>>>>Cc: James E.J. Bottomley; linux-scsi@vger.kernel.org;
>>>>security@kernel.org; Nico Golde; Fabian Yamaguchi
>>>>Subject: [patch] [SCSI] megaraid: missing bounds check in
>>>>mimd_to_kioc()
>>>>
>>>>pthru32->dataxferlen comes from the user so we need to check that it's
>>>>not too large so we don't overflow the buffer.
>>>>
>>>>Reported-by: Nico Golde <nico@ngolde.de>
>>>>Reported-by: Fabian Yamaguchi <fabs@goesec.de>
>>>>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>---
>>>>Please review this carefully because I have not tested it.
>>>>
>>>>diff --git a/drivers/scsi/megaraid/megaraid_mm.c
>>>>b/drivers/scsi/megaraid/megaraid_mm.c
>>>>index dfffd0f..a706927 100644
>>>>--- a/drivers/scsi/megaraid/megaraid_mm.c
>>>>+++ b/drivers/scsi/megaraid/megaraid_mm.c
>>>>@@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd,
>>mraid_mmadp_t
>>>>*adp, uioc_t *kioc)
>>>>
>>>>       pthru32->dataxferaddr   = kioc->buf_paddr;
>>>>       if (kioc->data_dir & UIOC_WR) {
>>>>+              if (pthru32->dataxferlen > kioc->xferlen)
>>>>+                      return -EINVAL;
>>>>               if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
>>>>                                               pthru32->dataxferlen)) {
>>>>                       return (-EFAULT);
>>>
>>> Acked-by: Sumit Saxena <sumit.saxena@lsi.com>
>>>
>>> Sumit
>>>
>>
>>Thanks for the Ack. Who normally picks patches for this area?
>>
>>-Kees
>>
> James Bottomley(Linux SCSI subsystem maintainer) should pick this patch.

Okay, thanks. James, can you pick up this patch as well?
https://lkml.org/lkml/2013/12/18/477

Thanks,

-Kees

>
> Sumit
>>--
>>Kees Cook
>>Chrome OS Security
>



-- 
Kees Cook
Chrome OS Security