[2.6.7-rc1-mm1] lp int copy_to_user replaced

[2.6.7-rc1-mm1] lp int copy_to_user replaced

[FabF]

[-- Attachment #1: Type: text/plain, Size: 119 bytes --]

Andrew,

	Here's a patch to have standard __put_user for integer transfers in lp
driver.Is it correct ?

Regards,
FabF

[-- Attachment #2: lp1.diff --]
[-- Type: text/x-patch, Size: 1250 bytes --]

diff -Naur orig/drivers/char/lp.c edited/drivers/char/lp.c
--- orig/drivers/char/lp.c	2004-05-10 04:31:58.000000000 +0200
+++ edited/drivers/char/lp.c	2004-05-27 19:28:12.581860488 +0200
@@ -31,6 +31,8 @@
  * Obsoleted and removed all the lowlevel stuff implemented in the last
  * month to use the IEEE1284 functions (that handle the _new_ compatibilty
  * mode fine).
+ *
+ * 27-MAY-2004  Replace int copy_to_user with put_user (Fabian Frederick)
  */
 
 /* This driver should, in theory, work with any parallel port that has an
@@ -603,16 +605,14 @@
 			return -EINVAL;
 			break;
 		case LPGETIRQ:
-			if (copy_to_user((int *) arg, &LP_IRQ(minor),
-					sizeof(int)))
+			if (__put_user(LP_IRQ(minor), (int *) arg))
 				return -EFAULT;
 			break;
 		case LPGETSTATUS:
 			lp_claim_parport_or_block (&lp_table[minor]);
 			status = r_str(minor);
 			lp_release_parport (&lp_table[minor]);
-
-			if (copy_to_user((int *) arg, &status, sizeof(int)))
+			if (__put_user(status, (int *) arg))
 				return -EFAULT;
 			break;
 		case LPRESET:
@@ -630,7 +630,7 @@
 #endif
  		case LPGETFLAGS:
  			status = LP_F(minor);
-			if (copy_to_user((int *) arg, &status, sizeof(int)))
+			if (__put_user(status, (int *) arg))
 				return -EFAULT;
 			break;
 

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On Thu, 2004-05-27 at 19:32, FabF wrote:
> Andrew,
> 
> 	Here's a patch to have standard __put_user for integer transfers in lp
> driver.Is it correct ?

no it's not. You need to use put_user() not __put_user() at least, to
make sure the destination address is checked to not be in kernel
space...


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[Chris Wright]

* FabF (fabian.frederick@skynet.be) wrote:
> Andrew,
> 
> 	Here's a patch to have standard __put_user for integer transfers in lp
> driver.Is it correct ?

No, you've removed the security check but using __put_user.  put_user
maintains the access_ok() check.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[FabF]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

Here it is.

FabF

On Thu, 2004-05-27 at 19:39, Arjan van de Ven wrote:
> On Thu, 2004-05-27 at 19:32, FabF wrote:
> > Andrew,
> > 
> > 	Here's a patch to have standard __put_user for integer transfers in lp
> > driver.Is it correct ?
> 
> no it's not. You need to use put_user() not __put_user() at least, to
> make sure the destination address is checked to not be in kernel
> space...
> 

[-- Attachment #2: lp2.diff --]
[-- Type: text/x-patch, Size: 1244 bytes --]

diff -Naur orig/drivers/char/lp.c edited/drivers/char/lp.c
--- orig/drivers/char/lp.c	2004-05-10 04:31:58.000000000 +0200
+++ edited/drivers/char/lp.c	2004-05-27 19:43:43.649316720 +0200
@@ -31,6 +31,8 @@
  * Obsoleted and removed all the lowlevel stuff implemented in the last
  * month to use the IEEE1284 functions (that handle the _new_ compatibilty
  * mode fine).
+ *
+ * 27-MAY-2004  Replace int copy_to_user with put_user (Fabian Frederick)
  */
 
 /* This driver should, in theory, work with any parallel port that has an
@@ -603,16 +605,14 @@
 			return -EINVAL;
 			break;
 		case LPGETIRQ:
-			if (copy_to_user((int *) arg, &LP_IRQ(minor),
-					sizeof(int)))
+			if (put_user(LP_IRQ(minor), (int *) arg))
 				return -EFAULT;
 			break;
 		case LPGETSTATUS:
 			lp_claim_parport_or_block (&lp_table[minor]);
 			status = r_str(minor);
 			lp_release_parport (&lp_table[minor]);
-
-			if (copy_to_user((int *) arg, &status, sizeof(int)))
+			if (put_user(status, (int *) arg))
 				return -EFAULT;
 			break;
 		case LPRESET:
@@ -630,7 +630,7 @@
 #endif
  		case LPGETFLAGS:
  			status = LP_F(minor);
-			if (copy_to_user((int *) arg, &status, sizeof(int)))
+			if (put_user(status, (int *) arg))
 				return -EFAULT;
 			break;
 

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[viro]

On Thu, May 27, 2004 at 07:32:08PM +0200, FabF wrote:
> Andrew,
> 
> 	Here's a patch to have standard __put_user for integer transfers in lp
> driver.Is it correct ?

What the hell for?  copy_to_user()/copy_from_user() is perfectly OK here.

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[FabF]

On Thu, 2004-05-27 at 20:01, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Thu, May 27, 2004 at 07:32:08PM +0200, FabF wrote:
> > Andrew,
> > 
> > 	Here's a patch to have standard __put_user for integer transfers in lp
> > driver.Is it correct ?
> 
> What the hell for?  copy_to_user()/copy_from_user() is perfectly OK here.
> 
And why the hell use generic functions when we have neat small type
exchange macros ?

FabF

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[viro]

On Thu, May 27, 2004 at 08:06:55PM +0200, FabF wrote:
> On Thu, 2004-05-27 at 20:01, viro@parcelfarce.linux.theplanet.co.uk
> wrote:
> > On Thu, May 27, 2004 at 07:32:08PM +0200, FabF wrote:
> > > Andrew,
> > > 
> > > 	Here's a patch to have standard __put_user for integer transfers in lp
> > > driver.Is it correct ?
> > 
> > What the hell for?  copy_to_user()/copy_from_user() is perfectly OK here.
> > 
> And why the hell use generic functions when we have neat small type
> exchange macros ?

We have different definitions of "neat", apparently.

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[Davide Libenzi]

On Thu, 27 May 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:

> On Thu, May 27, 2004 at 07:32:08PM +0200, FabF wrote:
> > Andrew,
> > 
> > 	Here's a patch to have standard __put_user for integer transfers in lp
> > driver.Is it correct ?
> 
> What the hell for?  copy_to_user()/copy_from_user() is perfectly OK here.

Andrew love __put_user() against __copy_to_user() :-) We had a similar 
discussion last week.



- Davide

Re: [2.6.7-rc1-mm1] lp int copy_to_user replaced

[Andrew Morton]

FabF <fabian.frederick@skynet.be> wrote:
>
>  On Thu, 2004-05-27 at 20:01, viro@parcelfarce.linux.theplanet.co.uk
>  wrote:
>  > On Thu, May 27, 2004 at 07:32:08PM +0200, FabF wrote:
>  > > Andrew,
>  > > 
>  > > 	Here's a patch to have standard __put_user for integer transfers in lp
>  > > driver.Is it correct ?
>  > 
>  > What the hell for?  copy_to_user()/copy_from_user() is perfectly OK here.
>  > 
>  And why the hell use generic functions when we have neat small type
>  exchange macros ?

copy_*_user() has special-case code to handle 1, 2 and 4-byte copies, so
your patch should make no difference on x86.  Other architectures do not
have that optimisation.

So the patch is a tiny optimisation for some architectures.  However, it's
of very small benefit - we could make a million such changes.  Would prefer
more substantial things at this time, please.