Re: [PATCH] add missing checks of __copy_to_user return value ini2o_config.c

[Jesper Juhl]

On Tue, 5 Oct 2004, Andrew Morton wrote:

> Jesper Juhl <juhl-lkml@xxxxxx> 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@xxxxxx>

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)


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