* [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