* [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
@ 2004-10-05 21:43 Jesper Juhl
2004-10-05 22:21 ` Andrew Morton
2004-10-06 10:41 ` Christoph Hellwig
0 siblings, 2 replies; 9+ messages in thread
From: Jesper Juhl @ 2004-10-05 21:43 UTC (permalink / raw)
To: linux-kernel; +Cc: Markus Lidel, Alan Cox
This patch fixes up the following :
CC drivers/message/i2o/i2o_config.o
include/asm/uaccess.h: In function `i2o_cfg_getiops':
drivers/message/i2o/i2o_config.c:190: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
include/asm/uaccess.h: In function `i2o_cfg_swul':
drivers/message/i2o/i2o_config.c:477: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
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-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;
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);
return 0;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-05 21:43 [PATCH] add missing checks of __copy_to_user return value in i2o_config.c Jesper Juhl
@ 2004-10-05 22:21 ` Andrew Morton
2004-10-05 22:34 ` Jesper Juhl
2004-10-06 10:41 ` Christoph Hellwig
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2004-10-05 22:21 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-kernel, markus.lidel, alan
Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
> - __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);
>
Obvious leak.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-05 22:21 ` Andrew Morton
@ 2004-10-05 22:34 ` Jesper Juhl
2004-10-05 22:43 ` Andrew Morton
0 siblings, 1 reply; 9+ messages in thread
From: Jesper Juhl @ 2004-10-05 22:34 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, markus.lidel, alan
On Tue, 5 Oct 2004, Andrew Morton wrote:
> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> >
> > - __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);
> >
>
> Obvious leak.
>
Whoops, so sorry about that, I must be sleeping. Thank you for your
vigilance :)
Here's a better patch.
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 00:29:30.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;
return 0;
};
@@ -474,7 +475,11 @@ 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)) {
+ i2o_dma_free(&c->pdev->dev, &buffer);
+ return -EFAULT;
+ }
+
i2o_dma_free(&c->pdev->dev, &buffer);
return 0;
Could probably be done a little nicer for the entire function with a
"retval" variable and some goto's here and there, but I opted for the
simple solution.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-05 22:34 ` Jesper Juhl
@ 2004-10-05 22:43 ` Andrew Morton
2004-10-05 23:11 ` Jesper Juhl
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2004-10-05 22:43 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-kernel, markus.lidel, alan
Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
> - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize)) {
> + i2o_dma_free(&c->pdev->dev, &buffer);
> + return -EFAULT;
> + }
> +
> i2o_dma_free(&c->pdev->dev, &buffer);
Please try to avoid more than a single return statement per function.
Also, please try to avoid duplicating code such as the above - it's a
maintainance problem is nothing else.
Like this:
--- 25/drivers/message/i2o/i2o_config.c~add-missing-checks-of-__copy_to_user-return-value-in Tue Oct 5 15:41:04 2004
+++ 25-akpm/drivers/message/i2o/i2o_config.c Tue Oct 5 15:42:17 2004
@@ -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;
return 0;
};
@@ -416,6 +417,7 @@ static int i2o_cfg_swul(unsigned long ar
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int ret;
if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
return -EFAULT;
@@ -474,10 +476,11 @@ static int i2o_cfg_swul(unsigned long ar
return status;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ ret = 0;
+ if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ ret = -EFAULT;
i2o_dma_free(&c->pdev->dev, &buffer);
-
- return 0;
+ return ret;
};
static int i2o_cfg_swdel(unsigned long arg)
_
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-05 22:43 ` Andrew Morton
@ 2004-10-05 23:11 ` Jesper Juhl
0 siblings, 0 replies; 9+ messages in thread
From: Jesper Juhl @ 2004-10-05 23:11 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, markus.lidel, alan
On Tue, 5 Oct 2004, Andrew Morton wrote:
> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> >
> > - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> > + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize)) {
> > + i2o_dma_free(&c->pdev->dev, &buffer);
> > + return -EFAULT;
> > + }
> > +
> > i2o_dma_free(&c->pdev->dev, &buffer);
>
> Please try to avoid more than a single return statement per function.
>
Will do. But this function already had a ton of return statements so I
assumed one more wouldn't make any real difference - I'll be sure to give
that more thought in the future.
> Also, please try to avoid duplicating code such as the above - it's a
> maintainance problem is nothing else.
>
> Like this:
>
> - __copy_to_user(kxfer.buf, buffer.virt, fragsize);
> + ret = 0;
> + if (__copy_to_user(kxfer.buf, buffer.virt, fragsize))
> + ret = -EFAULT;
> i2o_dma_free(&c->pdev->dev, &buffer);
> -
> - return 0;
> + return ret;
> };
Yes, that's what I had in mind when I noted a prettier version could be
made with a retval variable, the goto's I hinted at where intended to get
rid of the many return statements and create a single exit point for the
function. Your point about maintainability is noted.
How about just doing it all as in this patch below ?
I considered adding
return_nomem:
ret = -ENOMEM;
goto return_ret;
etc at the bottom as well, for the other return statements to use, but
that would be a lot of labels and gotos at the end, seems cleaner not to -
comments?
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 01:05:04.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;
return 0;
};
@@ -416,24 +417,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;
+ goto return_fault;
c = i2o_find_iop(kxfer.iop);
if (!c)
@@ -474,10 +476,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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-05 21:43 [PATCH] add missing checks of __copy_to_user return value in i2o_config.c Jesper Juhl
2004-10-05 22:21 ` Andrew Morton
@ 2004-10-06 10:41 ` Christoph Hellwig
2004-10-06 20:11 ` Jesper Juhl
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2004-10-06 10:41 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-kernel, Markus Lidel, Alan Cox
On Tue, Oct 05, 2004 at 11:43:54PM +0200, Jesper Juhl wrote:
>
> This patch fixes up the following :
>
> CC drivers/message/i2o/i2o_config.o
> include/asm/uaccess.h: In function `i2o_cfg_getiops':
> drivers/message/i2o/i2o_config.c:190: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
> include/asm/uaccess.h: In function `i2o_cfg_swul':
> drivers/message/i2o/i2o_config.c:477: warning: ignoring return value of `__copy_to_user', declared with attribute warn_unused_result
>
> 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-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.
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-06 10:41 ` Christoph Hellwig
@ 2004-10-06 20:11 ` Jesper Juhl
2004-10-07 21:01 ` Markus Lidel
0 siblings, 1 reply; 9+ messages in thread
From: Jesper Juhl @ 2004-10-06 20:11 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-kernel, Markus Lidel, Alan Cox, Andrew Morton
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-06 20:11 ` Jesper Juhl
@ 2004-10-07 21:01 ` Markus Lidel
2004-10-08 0:23 ` Jesper Juhl
0 siblings, 1 reply; 9+ messages in thread
From: Markus Lidel @ 2004-10-07 21:01 UTC (permalink / raw)
To: Jesper Juhl; +Cc: Christoph Hellwig, linux-kernel, Alan Cox, Andrew Morton
[-- Attachment #1: Type: text/plain, Size: 522 bytes --]
Hello,
thanks for fixing i2o_config. In the meantime i've also made a patch,
which tries to reduce the many return's too.
Note: i've not touched the passthru stuff yet.
Please let me know, which you think of it.
Best regards,
Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)
Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany
Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11
E-Mail: Markus.Lidel@shadowconnect.com
URL: http://www.shadowconnect.com
[-- Attachment #2: i2o_config-copy_user-fix.patch --]
[-- Type: text/x-patch, Size: 12274 bytes --]
--- a/drivers/message/i2o/i2o_config.c 2004-09-24 11:05:12.044972000 +0200
+++ b/drivers/message/i2o/i2o_config.c 2004-10-07 22:49:59.786543596 +0200
@@ -178,18 +178,17 @@
struct i2o_controller *c;
u8 __user *user_iop_table = (void __user *)arg;
u8 tmp[MAX_I2O_CONTROLLERS];
+ int rc = 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))
+ rc = -EFAULT;
- return 0;
+ return rc;
};
static int i2o_cfg_gethrt(unsigned long arg)
@@ -200,20 +199,28 @@
i2o_hrt *hrt;
int len;
u32 reslen;
- int ret = 0;
+ int rc = 0;
- if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct)))
- return -EFAULT;
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(reslen, kcmd.reslen) < 0)
- return -EFAULT;
+ if (get_user(reslen, kcmd.reslen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (kcmd.resbuf == NULL)
- return -EFAULT;
+ if (kcmd.resbuf == NULL) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kcmd.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
hrt = (i2o_hrt *) c->hrt.virt;
@@ -221,12 +228,16 @@
/* We did a get user...so assuming mem is ok...is this bad? */
put_user(len, kcmd.reslen);
- if (len > reslen)
- ret = -ENOBUFS;
+ if (len > reslen) {
+ rc = -ENOBUFS;
+ goto exit;
+ }
+
if (copy_to_user(kcmd.resbuf, (void *)hrt, len))
- ret = -EFAULT;
+ rc = -EFAULT;
- return ret;
+exit:
+ return rc;
};
static int i2o_cfg_getlct(unsigned long arg)
@@ -236,37 +247,48 @@
struct i2o_cmd_hrtlct kcmd;
i2o_lct *lct;
int len;
- int ret = 0;
+ int rc = 0;
u32 reslen;
- if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct)))
- return -EFAULT;
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(reslen, kcmd.reslen) < 0)
- return -EFAULT;
+ if (get_user(reslen, kcmd.reslen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (kcmd.resbuf == NULL)
- return -EFAULT;
+ if (kcmd.resbuf == NULL) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kcmd.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
lct = (i2o_lct *) c->lct;
len = (unsigned int)lct->table_size << 2;
put_user(len, kcmd.reslen);
- if (len > reslen)
- ret = -ENOBUFS;
- else if (copy_to_user(kcmd.resbuf, lct, len))
- ret = -EFAULT;
+ if (len > reslen) {
+ rc = -ENOBUFS;
+ goto exit;
+ }
- return ret;
+ if (copy_to_user(kcmd.resbuf, lct, len))
+ rc = -EFAULT;
+
+exit:
+ return rc;
};
static int i2o_cfg_parms(unsigned long arg, unsigned int type)
{
- int ret = 0;
struct i2o_controller *c;
struct i2o_device *dev;
struct i2o_cmd_psetget __user *cmd =
@@ -276,31 +298,42 @@
u8 *ops;
u8 *res;
int len = 0;
+ int rc = 0;
u32 i2o_cmd = (type == I2OPARMGET ?
I2O_CMD_UTIL_PARAMS_GET : I2O_CMD_UTIL_PARAMS_SET);
- if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_psetget)))
- return -EFAULT;
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_psetget))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(reslen, kcmd.reslen))
- return -EFAULT;
+ if (get_user(reslen, kcmd.reslen)) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kcmd.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
dev = i2o_iop_find_device(c, kcmd.tid);
- if (!dev)
- return -ENXIO;
+ if (!dev) {
+ rc = -ENXIO;
+ goto exit;
+ }
ops = (u8 *) kmalloc(kcmd.oplen, GFP_KERNEL);
- if (!ops)
- return -ENOMEM;
+ if (!ops) {
+ rc = -ENOMEM;
+ goto exit;
+ }
if (copy_from_user(ops, kcmd.opbuf, kcmd.oplen)) {
- kfree(ops);
- return -EFAULT;
+ rc = -EFAULT;
+ goto cleanup_ops;
}
/*
@@ -309,27 +342,33 @@
*/
res = (u8 *) kmalloc(65536, GFP_KERNEL);
if (!res) {
- kfree(ops);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto cleanup_ops;
}
len = i2o_parm_issue(dev, i2o_cmd, ops, kcmd.oplen, res, 65536);
- kfree(ops);
-
if (len < 0) {
- kfree(res);
- return -EAGAIN;
+ rc = -EAGAIN;
+ goto cleanup_res;
}
put_user(len, kcmd.reslen);
- if (len > reslen)
- ret = -ENOBUFS;
- else if (copy_to_user(kcmd.resbuf, res, len))
- ret = -EFAULT;
+ if (len > reslen) {
+ rc = -ENOBUFS;
+ goto cleanup_res;
+ }
+
+ if (copy_to_user(kcmd.resbuf, res, len))
+ rc = -EFAULT;
+cleanup_res:
kfree(res);
- return ret;
+cleanup_ops:
+ kfree(ops);
+
+exit:
+ return rc;
};
static int i2o_cfg_swdl(unsigned long arg)
@@ -342,39 +381,57 @@
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int rc = 0;
- if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ if (get_user(swlen, kxfer.swlen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(maxfrag, kxfer.maxfrag) < 0)
- return -EFAULT;
+ if (get_user(maxfrag, kxfer.maxfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(curfrag, kxfer.curfrag) < 0)
- return -EFAULT;
+ if (get_user(curfrag, kxfer.curfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
if (curfrag == maxfrag)
fragsize = swlen - (maxfrag - 1) * 8192;
- if (!kxfer.buf || !access_ok(VERIFY_READ, kxfer.buf, fragsize))
- return -EFAULT;
+ if (!kxfer.buf) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kxfer.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
if (i2o_dma_alloc(&c->pdev->dev, &buffer, fragsize, GFP_KERNEL)) {
- i2o_msg_nop(c, m);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto nop_msg;
}
- __copy_from_user(buffer.virt, kxfer.buf, fragsize);
+ if(copy_from_user(buffer.virt, kxfer.buf, fragsize)) {
+ rc = -EFAULT;
+ goto cleanup_buffer;
+ }
writel(NINE_WORD_MSG_SIZE | SGL_OFFSET_7, &msg->u.head[0]);
writel(I2O_CMD_SW_DOWNLOAD << 24 | HOST_TID << 12 | ADAPTER_TID,
@@ -400,10 +457,19 @@
printk(KERN_INFO
"i2o_config: swdl failed, DetailedStatus = %d\n",
status);
- return status;
+ rc = status;
}
- return 0;
+exit:
+ return rc;
+
+cleanup_buffer:
+ i2o_dma_free(&c->pdev->dev, &buffer);
+
+nop_msg:
+ i2o_msg_nop(c, m);
+
+ return rc;
};
static int i2o_cfg_swul(unsigned long arg)
@@ -416,36 +482,51 @@
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int rc = 0;
- if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ if (get_user(swlen, kxfer.swlen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(maxfrag, kxfer.maxfrag) < 0)
- return -EFAULT;
+ if (get_user(maxfrag, kxfer.maxfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(curfrag, kxfer.curfrag) < 0)
- return -EFAULT;
+ if (get_user(curfrag, kxfer.curfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
if (curfrag == maxfrag)
fragsize = swlen - (maxfrag - 1) * 8192;
- if (!kxfer.buf || !access_ok(VERIFY_WRITE, kxfer.buf, fragsize))
- return -EFAULT;
+ if (!kxfer.buf) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kxfer.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
if (i2o_dma_alloc(&c->pdev->dev, &buffer, fragsize, GFP_KERNEL)) {
- i2o_msg_nop(c, m);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto nop_msg;
}
writel(NINE_WORD_MSG_SIZE | SGL_OFFSET_7, &msg->u.head[0]);
@@ -471,13 +552,22 @@
printk(KERN_INFO
"i2o_config: swul failed, DetailedStatus = %d\n",
status);
- return status;
+ rc = status;
+ goto exit;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ if(copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ rc = -EFAULT;
+
i2o_dma_free(&c->pdev->dev, &buffer);
- return 0;
+exit:
+ return rc;
+
+nop_msg:
+ i2o_msg_nop(c, m);
+
+ return rc;
};
static int i2o_cfg_swdel(unsigned long arg)
@@ -489,20 +579,29 @@
u32 m;
unsigned int swlen;
int token;
+ int rc = 0;
- if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ if (get_user(swlen, kxfer.swlen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kxfer.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
writel(SEVEN_WORD_MSG_SIZE | SGL_OFFSET_0, &msg->u.head[0]);
writel(I2O_CMD_SW_REMOVE << 24 | HOST_TID << 12 | ADAPTER_TID,
@@ -520,10 +619,11 @@
printk(KERN_INFO
"i2o_config: swdel failed, DetailedStatus = %d\n",
token);
- return -ETIMEDOUT;
+ rc = -ETIMEDOUT;
}
- return 0;
+exit:
+ return rc;
};
static int i2o_cfg_validate(unsigned long arg)
@@ -533,14 +633,19 @@
struct i2o_message *msg;
u32 m;
struct i2o_controller *c;
+ int rc = 0;
c = i2o_find_iop(iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
writel(FOUR_WORD_MSG_SIZE | SGL_OFFSET_0, &msg->u.head[0]);
writel(I2O_CMD_CONFIG_VALIDATE << 24 | HOST_TID << 12 | iop,
@@ -553,10 +658,11 @@
if (token != I2O_POST_WAIT_OK) {
printk(KERN_INFO "Can't validate configuration, ErrorStatus = "
"%d\n", token);
- return -ETIMEDOUT;
+ rc = -ETIMEDOUT;
}
- return 0;
+exit:
+ return rc;
};
static int i2o_cfg_evt_reg(unsigned long arg, struct file *fp)
@@ -567,23 +673,32 @@
struct i2o_evt_id kdesc;
struct i2o_controller *c;
struct i2o_device *d;
+ int rc = 0;
- if (copy_from_user(&kdesc, pdesc, sizeof(struct i2o_evt_id)))
- return -EFAULT;
+ if (copy_from_user(&kdesc, pdesc, sizeof(struct i2o_evt_id))) {
+ rc = -EFAULT;
+ goto exit;
+ }
/* IOP exists? */
c = i2o_find_iop(kdesc.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
/* Device exists? */
d = i2o_iop_find_device(c, kdesc.tid);
- if (!d)
- return -ENODEV;
+ if (!d) {
+ rc = -ENODEV;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
writel(FOUR_WORD_MSG_SIZE | SGL_OFFSET_0, &msg->u.head[0]);
writel(I2O_CMD_UTIL_EVT_REGISTER << 24 | HOST_TID << 12 | kdesc.tid,
@@ -594,7 +709,8 @@
i2o_msg_post(c, m);
- return 0;
+exit:
+ return rc;
}
static int i2o_cfg_evt_get(unsigned long arg, struct file *fp)
@@ -603,13 +719,16 @@
struct i2o_evt_get __user *uget = (struct i2o_evt_get __user *)arg;
struct i2o_evt_get kget;
unsigned long flags;
+ int rc = 0;
for (p = open_files; p; p = p->next)
if (p->q_id == (ulong) fp->private_data)
break;
- if (!p->q_len)
- return -ENOENT;
+ if (!p->q_len) {
+ rc = -ENOENT;
+ goto exit;
+ }
memcpy(&kget.info, &p->event_q[p->q_out], sizeof(struct i2o_evt_info));
MODINC(p->q_out, I2O_EVT_Q_LEN);
@@ -620,8 +739,10 @@
spin_unlock_irqrestore(&i2o_config_lock, flags);
if (copy_to_user(uget, &kget, sizeof(struct i2o_evt_get)))
- return -EFAULT;
- return 0;
+ rc = -EFAULT;
+
+exit:
+ return rc;
}
#if BITS_PER_LONG == 64
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
2004-10-07 21:01 ` Markus Lidel
@ 2004-10-08 0:23 ` Jesper Juhl
0 siblings, 0 replies; 9+ messages in thread
From: Jesper Juhl @ 2004-10-08 0:23 UTC (permalink / raw)
To: Markus Lidel; +Cc: Christoph Hellwig, linux-kernel, Alan Cox, Andrew Morton
On Thu, 7 Oct 2004, Markus Lidel wrote:
> Hello,
> thanks for fixing i2o_config.
You're welcome.
> In the meantime i've also made a patch, which
> tries to reduce the many return's too.
> Note: i've not touched the passthru stuff yet.
> Please let me know, which you think of it.
> --- a/drivers/message/i2o/i2o_config.c 2004-09-24 11:05:12.044972000 +0200
> +++ b/drivers/message/i2o/i2o_config.c 2004-10-07 22:49:59.786543596 +0200
> @@ -178,18 +178,17 @@
> struct i2o_controller *c;
> u8 __user *user_iop_table = (void __user *)arg;
> u8 tmp[MAX_I2O_CONTROLLERS];
> + int rc = 0;
Purely personal preference, I think "ret" is a more intuitive name for
that variable.
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct))) {
+ rc = -EFAULT;
+ goto exit;
+ }
[...]
+exit:
+ return rc;
};
I prefer the
if (foo)
goto return_fault;
[...]
return_ret:
return ret;
return_fault:
ret = -EFAULT;
goto return_ret;
variant.
Sure, your way results in just a single mov and a single jmp when
the branch is taken, but, my way has only a single jmp at each branch,
then a mov and an additional jmp at the end, so for a single branch your
way wins with
1 mov + 1 jmp vs my 2 jmp's and 1 mov.
but, when you have a lot those branches my way generates less code. At 5
branches you have 5 mov + 5 jmp while I'll have 1 mov + 6 jmp.
Also, my way takes up less space in the source, so more code fits on
screen.
I doubt it matters much since this doesn't look like a very hot code path,
but I prefer the version that generates the smaller code and takes up less
space in the source.
Apart from those nitpicks it looks good to me :)
--
Jesper Juhl
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2004-10-08 0:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-10-05 21:43 [PATCH] add missing checks of __copy_to_user return value in i2o_config.c Jesper Juhl
2004-10-05 22:21 ` Andrew Morton
2004-10-05 22:34 ` Jesper Juhl
2004-10-05 22:43 ` Andrew Morton
2004-10-05 23:11 ` Jesper Juhl
2004-10-06 10:41 ` Christoph Hellwig
2004-10-06 20:11 ` Jesper Juhl
2004-10-07 21:01 ` Markus Lidel
2004-10-08 0:23 ` Jesper Juhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox