>From 90d75780b319153d050bb1c0f3f38ce054775615 Mon Sep 17 00:00:00 2001
From: Arjan van de Ven <[email protected]>
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 <[email protected]>
---
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
On Wed, 30 Sep 2009 13:17:09 +0200
Arjan van de Ven <[email protected]> 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?
On Wed, 30 Sep 2009 13:17:09 +0200
Arjan van de Ven <[email protected]> wrote:
>
> >From 90d75780b319153d050bb1c0f3f38ce054775615 Mon Sep 17 00:00:00 2001
> From: Arjan van de Ven <[email protected]>
> 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 <[email protected]>
> ---
> 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 */
_
On Wed, 30 Sep 2009 16:13:16 -0700
Andrew Morton <[email protected]> wrote:
> On Wed, 30 Sep 2009 13:17:09 +0200
> Arjan van de Ven <[email protected]> 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