[PATCH] floppy: Add an extra bound check on ioctl arguments

[PATCH] floppy: Add an extra bound check on ioctl arguments

[Arjan van de Ven]

>From 90d75780b319153d050bb1c0f3f38ce054775615 Mon Sep 17 00:00:00 2001
From: Arjan van de Ven <arjan@linux.intel.com>
Date: Wed, 30 Sep 2009 13:13:59 +0200
Subject: [PATCH] floppy: Add an extra bound check on ioctl arguments

gcc is not convinced that the floppy.c ioctl has sufficient bound checks,
and frankly, as a human I have a hard time proving the same more or less
(the size comes from the ioctl argument. humpf. maybe. the code isn't
very nice)

This patch adds an explicit check to make 100% sure it's safe, better
than finding out later that there indeed was a gap.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/block/floppy.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 5c01f74..0f11a77 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3497,6 +3497,9 @@ static int fd_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
 	    ((cmd & 0x80) && !capable(CAP_SYS_ADMIN)))
 		return -EPERM;
 
+	if (size < 0 || size > sizeof(inparam))
+		return -EINVAL;
+
 	/* copyin */
 	CLEARSTRUCT(&inparam);
 	if (_IOC_DIR(cmd) & _IOC_WRITE)
-- 
1.6.2.5


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] floppy: Add an extra bound check on ioctl arguments

[Andrew Morton]

On Wed, 30 Sep 2009 13:17:09 +0200
Arjan van de Ven <arjan@infradead.org> wrote:

> gcc is not convinced that the floppy.c ioctl has sufficient bound checks,

gad.  You said "floppy" and "ioctl" in the same sentence.  Where angels
fear to tread.

It would be useful if you were to quote the gcc output in the changelog
please.  I assume that you're using some magical new gcc option or
something?

Re: [PATCH] floppy: Add an extra bound check on ioctl arguments

[Arjan van de Ven]

On Wed, 30 Sep 2009 16:13:16 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 30 Sep 2009 13:17:09 +0200
> Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > gcc is not convinced that the floppy.c ioctl has sufficient bound
> > checks,
> 
> gad.  You said "floppy" and "ioctl" in the same sentence.  Where
> angels fear to tread.

I have to admit I was very much not looking forward to working on this
warning, and made sure to wear protective clothing and that my
immunizations were up to date.
> 
> It would be useful if you were to quote the gcc output in the
> changelog please. 


In function ‘copy_from_user’,
    inlined from ‘fd_copyin’ at drivers/block/floppy.c:3080,
    inlined from ‘fd_ioctl’ at drivers/block/floppy.c:3503:
/home/arjan/linux/arch/x86/include/asm/uaccess_32.h:211:
warning: call to ‘copy_from_user_overflow’ declared with attribute
warning: copy_from_user buffer size is not provably correct

> I assume that you're using some magical new gcc
> option or something?

Not so much a new option, as using an option that has been there for a
while, and has been used extensively in userspace, just not yet in the
kernel. The patches are in the -tip tree, but if you want to take a
peek I can send them to you as well... they find some "interesting"
stuff.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] floppy: Add an extra bound check on ioctl arguments

[Andrew Morton]

On Wed, 30 Sep 2009 13:17:09 +0200
Arjan van de Ven <arjan@infradead.org> wrote:

> 
> >From 90d75780b319153d050bb1c0f3f38ce054775615 Mon Sep 17 00:00:00 2001
> From: Arjan van de Ven <arjan@linux.intel.com>
> Date: Wed, 30 Sep 2009 13:13:59 +0200
> Subject: [PATCH] floppy: Add an extra bound check on ioctl arguments
> 
> gcc is not convinced that the floppy.c ioctl has sufficient bound checks,
> and frankly, as a human I have a hard time proving the same more or less
> (the size comes from the ioctl argument. humpf. maybe. the code isn't
> very nice)
> 
> This patch adds an explicit check to make 100% sure it's safe, better
> than finding out later that there indeed was a gap.
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/block/floppy.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 5c01f74..0f11a77 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3497,6 +3497,9 @@ static int fd_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
>  	    ((cmd & 0x80) && !capable(CAP_SYS_ADMIN)))
>  		return -EPERM;
>  
> +	if (size < 0 || size > sizeof(inparam))
> +		return -EINVAL;

I suspect that if this ever happens, normalize_ioctl() has
malfunctioned and what we have is a kernel bug.

This?

--- a/drivers/block/floppy.c~floppy-add-an-extra-bound-check-on-ioctl-arguments-fix
+++ a/drivers/block/floppy.c
@@ -3497,7 +3497,7 @@ static int fd_ioctl(struct block_device 
 	    ((cmd & 0x80) && !capable(CAP_SYS_ADMIN)))
 		return -EPERM;
 
-	if (size < 0 || size > sizeof(inparam))
+	if (WARN_ON(size < 0 || size > sizeof(inparam)))
 		return -EINVAL;
 
 	/* copyin */
_