[uml-devel] [patch] uml: check length in exitcode_proc_write()

[uml-devel] [patch] uml: check length in exitcode_proc_write()

[Dan Carpenter]

We don't cap the size of buffer from the user so we could write past
the end of the array here.  Only root can write to this file.

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/arch/um/kernel/exitcode.c b/arch/um/kernel/exitcode.c
index 829df49..41ebbfe 100644
--- a/arch/um/kernel/exitcode.c
+++ b/arch/um/kernel/exitcode.c
@@ -40,9 +40,11 @@ static ssize_t exitcode_proc_write(struct file *file,
 		const char __user *buffer, size_t count, loff_t *pos)
 {
 	char *end, buf[sizeof("nnnnn\0")];
+	size_t size;
 	int tmp;
 
-	if (copy_from_user(buf, buffer, count))
+	size = min(count, sizeof(buf));
+	if (copy_from_user(buf, buffer, size))
 		return -EFAULT;
 
 	tmp = simple_strtol(buf, &end, 0);

------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [patch] uml: check length in exitcode_proc_write()

[Richard Weinberger]

Am 29.10.2013 20:06, schrieb Dan Carpenter:
> We don't cap the size of buffer from the user so we could write past
> the end of the array here.  Only root can write to this file.
> 
> Reported-by: Nico Golde <nico@ngolde.de>
> Reported-by: Fabian Yamaguchi <fabs@goesec.de>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks everyone!
Patch applied and an it's way to Linus' tree.

Thanks,
//richard

> diff --git a/arch/um/kernel/exitcode.c b/arch/um/kernel/exitcode.c
> index 829df49..41ebbfe 100644
> --- a/arch/um/kernel/exitcode.c
> +++ b/arch/um/kernel/exitcode.c
> @@ -40,9 +40,11 @@ static ssize_t exitcode_proc_write(struct file *file,
>  		const char __user *buffer, size_t count, loff_t *pos)
>  {
>  	char *end, buf[sizeof("nnnnn\0")];
> +	size_t size;
>  	int tmp;
>  
> -	if (copy_from_user(buf, buffer, count))
> +	size = min(count, sizeof(buf));
> +	if (copy_from_user(buf, buffer, size))
>  		return -EFAULT;
>  
>  	tmp = simple_strtol(buf, &end, 0);
> 


------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

[patch] libertas: potential oops in debugfs

[Dan Carpenter]

If we do a zero size write then it will oops.  This can only be
triggered by root.

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/net/wireless/libertas/debugfs.c b/drivers/net/wireless/libertas/debugfs.c
index 668dd27..a148f14 100644
--- a/drivers/net/wireless/libertas/debugfs.c
+++ b/drivers/net/wireless/libertas/debugfs.c
@@ -913,6 +913,9 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
 	char *p2;
 	struct debug_data *d = f->private_data;
 
+	if (cnt == 0)
+		return 0;
+
 	pdata = kmalloc(cnt, GFP_KERNEL);
 	if (pdata == NULL)
 		return 0;

Re: [patch] libertas: potential oops in debugfs

[Dan Carpenter]

On Tue, Oct 29, 2013 at 10:06:41PM +0300, Dan Carpenter wrote:
> If we do a zero size write then it will oops.  This can only be
> triggered by root.
> 

This one isn't right...  Sorry for that.  We do need to fix the while
loop.  I wasn't thinking.

I will resend.

regards,
dan carpenter

[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 v2] libertas: potential oops in debugfs

[Dan Carpenter]

If we do a zero size allocation then it will oops.  Also we can't be
sure the user passes us a NUL terminated string so I've added a
terminator.

This code can only be triggered by root.

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/net/wireless/libertas/debugfs.c b/drivers/net/wireless/libertas/debugfs.c
index 668dd27..1917348 100644
--- a/drivers/net/wireless/libertas/debugfs.c
+++ b/drivers/net/wireless/libertas/debugfs.c
@@ -913,7 +913,10 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
 	char *p2;
 	struct debug_data *d = f->private_data;
 
-	pdata = kmalloc(cnt, GFP_KERNEL);
+	if (cnt = 0)
+		return 0;
+
+	pdata = kmalloc(cnt + 1, GFP_KERNEL);
 	if (pdata = NULL)
 		return 0;
 
@@ -922,6 +925,7 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
 		kfree(pdata);
 		return 0;
 	}
+	pdata[cnt] = '\0';
 
 	p0 = pdata;
 	for (i = 0; i < num_of_items; i++) {

[patch v2] libertas: potential oops in debugfs

[Dan Carpenter]

If we do a zero size allocation then it will oops.  Also we can't be
sure the user passes us a NUL terminated string so I've added a
terminator.

This code can only be triggered by root.

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/net/wireless/libertas/debugfs.c b/drivers/net/wireless/libertas/debugfs.c
index 668dd27..1917348 100644
--- a/drivers/net/wireless/libertas/debugfs.c
+++ b/drivers/net/wireless/libertas/debugfs.c
@@ -913,7 +913,10 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
 	char *p2;
 	struct debug_data *d = f->private_data;
 
-	pdata = kmalloc(cnt, GFP_KERNEL);
+	if (cnt == 0)
+		return 0;
+
+	pdata = kmalloc(cnt + 1, GFP_KERNEL);
 	if (pdata == NULL)
 		return 0;
 
@@ -922,6 +925,7 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
 		kfree(pdata);
 		return 0;
 	}
+	pdata[cnt] = '\0';
 
 	p0 = pdata;
 	for (i = 0; i < num_of_items; i++) {

Re: [patch v2] libertas: potential oops in debugfs

[Dan Williams]

On Wed, 2013-10-30 at 20:12 +0300, Dan Carpenter wrote:
> If we do a zero size allocation then it will oops.  Also we can't be
> sure the user passes us a NUL terminated string so I've added a
> terminator.
> 
> This code can only be triggered by root.
> 
> Reported-by: Nico Golde <nico@ngolde.de>
> Reported-by: Fabian Yamaguchi <fabs@goesec.de>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Dan Williams <dcbw@redhat.com>

> 
> diff --git a/drivers/net/wireless/libertas/debugfs.c b/drivers/net/wireless/libertas/debugfs.c
> index 668dd27..1917348 100644
> --- a/drivers/net/wireless/libertas/debugfs.c
> +++ b/drivers/net/wireless/libertas/debugfs.c
> @@ -913,7 +913,10 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
>  	char *p2;
>  	struct debug_data *d = f->private_data;
>  
> -	pdata = kmalloc(cnt, GFP_KERNEL);
> +	if (cnt = 0)
> +		return 0;
> +
> +	pdata = kmalloc(cnt + 1, GFP_KERNEL);
>  	if (pdata = NULL)
>  		return 0;
>  
> @@ -922,6 +925,7 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
>  		kfree(pdata);
>  		return 0;
>  	}
> +	pdata[cnt] = '\0';
>  
>  	p0 = pdata;
>  	for (i = 0; i < num_of_items; i++) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v2] libertas: potential oops in debugfs

[Dan Williams]

On Wed, 2013-10-30 at 20:12 +0300, Dan Carpenter wrote:
> If we do a zero size allocation then it will oops.  Also we can't be
> sure the user passes us a NUL terminated string so I've added a
> terminator.
> 
> This code can only be triggered by root.
> 
> Reported-by: Nico Golde <nico@ngolde.de>
> Reported-by: Fabian Yamaguchi <fabs@goesec.de>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Dan Williams <dcbw@redhat.com>

> 
> diff --git a/drivers/net/wireless/libertas/debugfs.c b/drivers/net/wireless/libertas/debugfs.c
> index 668dd27..1917348 100644
> --- a/drivers/net/wireless/libertas/debugfs.c
> +++ b/drivers/net/wireless/libertas/debugfs.c
> @@ -913,7 +913,10 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
>  	char *p2;
>  	struct debug_data *d = f->private_data;
>  
> -	pdata = kmalloc(cnt, GFP_KERNEL);
> +	if (cnt == 0)
> +		return 0;
> +
> +	pdata = kmalloc(cnt + 1, GFP_KERNEL);
>  	if (pdata == NULL)
>  		return 0;
>  
> @@ -922,6 +925,7 @@ static ssize_t lbs_debugfs_write(struct file *f, const char __user *buf,
>  		kfree(pdata);
>  		return 0;
>  	}
> +	pdata[cnt] = '\0';
>  
>  	p0 = pdata;
>  	for (i = 0; i < num_of_items; i++) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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

[patch] xfs: underflow bug in xfs_attrlist_by_handle()

[Dan Carpenter]

If we allocate less than sizeof(struct attrlist) then we end up
corrupting memory or doing a ZERO_PTR_SIZE dereference.

This can only be triggered with CAP_SYS_ADMIN.

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/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 4d61340..33ad9a7 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -442,7 +442,8 @@ xfs_attrlist_by_handle(
 		return -XFS_ERROR(EPERM);
 	if (copy_from_user(&al_hreq, arg, sizeof(xfs_fsop_attrlist_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
-	if (al_hreq.buflen > XATTR_LIST_MAX)
+	if (al_hreq.buflen < sizeof(struct attrlist) ||
+	    al_hreq.buflen > XATTR_LIST_MAX)
 		return -XFS_ERROR(EINVAL);
 
 	/*
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index e8fb123..a7992f8 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -356,7 +356,8 @@ xfs_compat_attrlist_by_handle(
 	if (copy_from_user(&al_hreq, arg,
 			   sizeof(compat_xfs_fsop_attrlist_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
-	if (al_hreq.buflen > XATTR_LIST_MAX)
+	if (al_hreq.buflen < sizeof(struct attrlist) ||
+	    al_hreq.buflen > XATTR_LIST_MAX)
 		return -XFS_ERROR(EINVAL);
 
 	/*

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [patch] xfs: underflow bug in xfs_attrlist_by_handle()

[Dave Chinner]

On Thu, Oct 31, 2013 at 09:00:10PM +0300, Dan Carpenter wrote:
> If we allocate less than sizeof(struct attrlist) then we end up
> corrupting memory or doing a ZERO_PTR_SIZE dereference.
> 
> This can only be triggered with CAP_SYS_ADMIN.
> 
> 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/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 4d61340..33ad9a7 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -442,7 +442,8 @@ xfs_attrlist_by_handle(
>  		return -XFS_ERROR(EPERM);
>  	if (copy_from_user(&al_hreq, arg, sizeof(xfs_fsop_attrlist_handlereq_t)))
>  		return -XFS_ERROR(EFAULT);
> -	if (al_hreq.buflen > XATTR_LIST_MAX)
> +	if (al_hreq.buflen < sizeof(struct attrlist) ||
> +	    al_hreq.buflen > XATTR_LIST_MAX)
>  		return -XFS_ERROR(EINVAL);

Yup, that's not checked in xfs_attr_list(). Looks like these are the
only direct callers of xfs_attr_list(), and the other callers of
xfs_attr_list_int() don't appear to have the same issue. Good find!

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [patch] xfs: underflow bug in xfs_attrlist_by_handle()

[Ben Myers]

On Thu, Oct 31, 2013 at 09:00:10PM +0300, Dan Carpenter wrote:
> If we allocate less than sizeof(struct attrlist) then we end up
> corrupting memory or doing a ZERO_PTR_SIZE dereference.
> 
> This can only be triggered with CAP_SYS_ADMIN.
> 
> Reported-by: Nico Golde <nico@ngolde.de>
> Reported-by: Fabian Yamaguchi <fabs@goesec.de>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied.  Thanks Dan.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs