lkml.org 
[lkml]   [2004]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c

Updated patch at the end (fourth edition).


On Wed, 6 Oct 2004, Christoph Hellwig wrote:

> On Tue, Oct 05, 2004 at 11:43:54PM +0200, Jesper Juhl wrote:
> >
> > diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
> > --- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
> > +++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-05 23:32:43.000000000 +0200
> > @@ -187,7 +187,8 @@ static int i2o_cfg_getiops(unsigned long
> > list_for_each_entry(c, &i2o_controllers, list)
> > tmp[c->unit] = 1;
> >
> > - __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
> > + if (__copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
> > + return -EFAULT;
>
> should be copy_to_user (with return value checked) and the
> access_ok above should be removed.
>
Makes sense to me. No need to explicitly check when copy_to_user() will do
so for us.


> > return 0;
> > };
> > @@ -474,7 +475,9 @@ static int i2o_cfg_swul(unsigned long ar
> > return status;
> > }
> >
> > - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> > + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
> > + return -EFAULT;
> > +
> > i2o_dma_free(&c->pdev->dev, &buffer);
>
> you're adding a leak here,and again please use copy_to_user and remove
> the access_ok abov

Yeah, I goofed, Andrew already spotted that one and I send him an updated
patch (it's in this thread). Unfortunately my second patch also left
something to be desired, so I send off a third (didn't get any comment on
that one yet). After reading your comments I guess a fourth attempt at
this is required - how about something like the following ?

It does the following:

- gets rid of access_ok
- replaces __copy_to_user with copy_to_user that calls access_ok itself
- makes sure copy_to_user return value is checked and acted upon
- gets a bit closer to the goal of having only one exit point pr function
- doesn't leak resources and doesn't duplicate code

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -up linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c
--- linux-2.6.9-rc3-bk5-orig/drivers/message/i2o/i2o_config.c 2004-09-30 05:05:40.000000000 +0200
+++ linux-2.6.9-rc3-bk5/drivers/message/i2o/i2o_config.c 2004-10-06 21:58:01.000000000 +0200
@@ -178,18 +178,17 @@ static int i2o_cfg_getiops(unsigned long
struct i2o_controller *c;
u8 __user *user_iop_table = (void __user *)arg;
u8 tmp[MAX_I2O_CONTROLLERS];
+ int ret = 0;

memset(tmp, 0, MAX_I2O_CONTROLLERS);

- if (!access_ok(VERIFY_WRITE, user_iop_table, MAX_I2O_CONTROLLERS))
- return -EFAULT;
-
list_for_each_entry(c, &i2o_controllers, list)
tmp[c->unit] = 1;

- __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
+ if (copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
+ ret = -EFAULT;

- return 0;
+ return ret;
};

static int i2o_cfg_gethrt(unsigned long arg)
@@ -416,24 +415,25 @@ static int i2o_cfg_swul(unsigned long ar
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int ret = 0;

if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ goto return_fault;

if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ goto return_fault;

if (get_user(maxfrag, kxfer.maxfrag) < 0)
- return -EFAULT;
+ goto return_fault;

if (get_user(curfrag, kxfer.curfrag) < 0)
- return -EFAULT;
+ goto return_fault;

if (curfrag == maxfrag)
fragsize = swlen - (maxfrag - 1) * 8192;

- if (!kxfer.buf || !access_ok(VERIFY_WRITE, kxfer.buf, fragsize))
- return -EFAULT;
+ if (!kxfer.buf)
+ goto return_fault;

c = i2o_find_iop(kxfer.iop);
if (!c)
@@ -474,10 +474,16 @@ static int i2o_cfg_swul(unsigned long ar
return status;
}

- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ if (copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ ret = -EFAULT;
+
i2o_dma_free(&c->pdev->dev, &buffer);

- return 0;
+return_ret:
+ return ret;
+return_fault:
+ ret = -EFAULT;
+ goto return_ret;
};

static int i2o_cfg_swdel(unsigned long arg)

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:06    [W:0.082 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site