[PATCH 1/3] aacraid: Fix return code interpretation

[PATCH 1/3] aacraid: Fix return code interpretation

[Mark Haverkamp]

Received from Mark Salyzyn

clear_user return is 0 for success, the code fragment is written to
assume that it is the count of the number of bytes zero'd.

Signed-off-by: Mark Haverkamp <markh@osdl.org>
---
Applies to the scsi-misc-2.6 git tree.

--- scsi-misc-aac.orig/drivers/scsi/aacraid/linit.c	2006-06-08 09:37:43.000000000 -0700
+++ scsi-misc-aac/drivers/scsi/aacraid/linit.c	2006-06-08 09:38:40.000000000 -0700
@@ -564,7 +564,7 @@
 		
 		f = compat_alloc_user_space(sizeof(*f));
 		ret = 0;
-		if (clear_user(f, sizeof(*f)) != sizeof(*f))
+		if (clear_user(f, sizeof(*f)))
 			ret = -EFAULT;
 		if (copy_in_user(f, (void __user *)arg, sizeof(struct fib_ioctl) - sizeof(u32)))
 			ret = -EFAULT;

-- 
Mark Haverkamp <markh@osdl.org>

Re: [PATCH 1/3] aacraid: Fix return code interpretation

[Christoph Hellwig]

> @@ -564,7 +564,7 @@
>  		
>  		f = compat_alloc_user_space(sizeof(*f));
>  		ret = 0;
> -		if (clear_user(f, sizeof(*f)) != sizeof(*f))
> +		if (clear_user(f, sizeof(*f)))
>  			ret = -EFAULT;
>  		if (copy_in_user(f, (void __user *)arg, sizeof(struct fib_ioctl) - sizeof(u32)))
>  			ret = -EFAULT;
> 

just remove the clear_user call completely, it's not needed.

RE: [PATCH 1/3] aacraid: Fix return code interpretation

[Salyzyn, Mark]

Are you sure of this? The code that follows expects the end of the
structure to be cleared.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org] 
> Sent: Thursday, June 08, 2006 4:11 PM
> To: Mark Haverkamp
> Cc: James Bottomley; linux-scsi; Salyzyn, Mark
> Subject: Re: [PATCH 1/3] aacraid: Fix return code interpretation
> 
> 
> > @@ -564,7 +564,7 @@
> >  		
> >  		f = compat_alloc_user_space(sizeof(*f));
> >  		ret = 0;
> > -		if (clear_user(f, sizeof(*f)) != sizeof(*f))
> > +		if (clear_user(f, sizeof(*f)))
> >  			ret = -EFAULT;
> >  		if (copy_in_user(f, (void __user *)arg, 
> sizeof(struct fib_ioctl) - sizeof(u32)))
> >  			ret = -EFAULT;
> > 
> 
> just remove the clear_user call completely, it's not needed.
> 
> 

RE: [PATCH 1/3] aacraid: Fix return code interpretation

[Mark Haverkamp]

On Fri, 2006-06-09 at 08:00 -0400, Salyzyn, Mark wrote:
> Are you sure of this? The code that follows expects the end of the
> structure to be cleared.

Could you clarify?  It looks like copy_in_user copies one u32 less than
the structure size and leaves that last word uninitialized?  The last
element of fib_ioctl (fib) is a char pointer though.

I can't see where f.fib is initialized in next_adapter_fib even though
copy_to_user is called.  Even if clear_user is  called, doesn't that
mean that f.fib in next_adapter_fib will be only partially NULL, 

Mark.


> 
> Sincerely -- Mark Salyzyn
> 
> > -----Original Message-----
> > From: Christoph Hellwig [mailto:hch@infradead.org] 
> > Sent: Thursday, June 08, 2006 4:11 PM
> > To: Mark Haverkamp
> > Cc: James Bottomley; linux-scsi; Salyzyn, Mark
> > Subject: Re: [PATCH 1/3] aacraid: Fix return code interpretation
> > 
> > 
> > > @@ -564,7 +564,7 @@
> > >  		
> > >  		f = compat_alloc_user_space(sizeof(*f));
> > >  		ret = 0;
> > > -		if (clear_user(f, sizeof(*f)) != sizeof(*f))
> > > +		if (clear_user(f, sizeof(*f)))
> > >  			ret = -EFAULT;
> > >  		if (copy_in_user(f, (void __user *)arg, 
> > sizeof(struct fib_ioctl) - sizeof(u32)))
> > >  			ret = -EFAULT;
> > > 
> > 
> > just remove the clear_user call completely, it's not needed.
> > 
> > 
> 
-- 
Mark Haverkamp <markh@osdl.org>

RE: [PATCH 1/3] aacraid: Fix return code interpretation

[Salyzyn, Mark]

Yes, that is the intent. A 32 bit application is calling a 64 bit
driver. The pointers in the 32 bit application merely need the virtual
address upper word zero'd to pass themselves off as 64 bit virtual
pointers. Yes, the clear_user call could be replaced with an =0 on the
last element, but it was more generic to clear it out completely before
copying in the subset. copy_[in|out]_user works with the constructed 64
bit virtual pointer.

Sincerely -- Mark Salyzyn


> -----Original Message-----
> From: Mark Haverkamp [mailto:markh@osdl.org] 
> Sent: Friday, June 09, 2006 12:22 PM
> To: Salyzyn, Mark
> Cc: Christoph Hellwig; James Bottomley; linux-scsi
> Subject: RE: [PATCH 1/3] aacraid: Fix return code interpretation
> 
> 
> On Fri, 2006-06-09 at 08:00 -0400, Salyzyn, Mark wrote:
> > Are you sure of this? The code that follows expects the end of the
> > structure to be cleared.
> 
> Could you clarify?  It looks like copy_in_user copies one u32 
> less than
> the structure size and leaves that last word uninitialized?  The last
> element of fib_ioctl (fib) is a char pointer though.
> 
> I can't see where f.fib is initialized in next_adapter_fib even though
> copy_to_user is called.  Even if clear_user is  called, doesn't that
> mean that f.fib in next_adapter_fib will be only partially NULL, 
> 
> Mark.
> 
> 
> > 
> > Sincerely -- Mark Salyzyn
> > 
> > > -----Original Message-----
> > > From: Christoph Hellwig [mailto:hch@infradead.org] 
> > > Sent: Thursday, June 08, 2006 4:11 PM
> > > To: Mark Haverkamp
> > > Cc: James Bottomley; linux-scsi; Salyzyn, Mark
> > > Subject: Re: [PATCH 1/3] aacraid: Fix return code interpretation
> > > 
> > > 
> > > > @@ -564,7 +564,7 @@
> > > >  		
> > > >  		f = compat_alloc_user_space(sizeof(*f));
> > > >  		ret = 0;
> > > > -		if (clear_user(f, sizeof(*f)) != sizeof(*f))
> > > > +		if (clear_user(f, sizeof(*f)))
> > > >  			ret = -EFAULT;
> > > >  		if (copy_in_user(f, (void __user *)arg, 
> > > sizeof(struct fib_ioctl) - sizeof(u32)))
> > > >  			ret = -EFAULT;
> > > > 
> > > 
> > > just remove the clear_user call completely, it's not needed.
> > > 
> > > 
> > 
> -- 
> Mark Haverkamp <markh@osdl.org>
> 
>