TAKE 956783 - xfs_dm_getall_dmattr() doesn't check if the user buffer is at valid address

TAKE 956783 - xfs_dm_getall_dmattr() doesn't check if the user buffer is at valid address

[Vlad Apostolov]

No EFAULT error when dm_getall_dmattr() called with an invalid user 
buffer address.

Date:  Tue Nov 21 17:14:34 AEDT 2006
Workarea:  soarer.melbourne.sgi.com:/home/vapo/isms/linux-xfs-dmapi
Inspected by:  donaldd
Author:  vapo

The following file(s) were checked into:
  longdrop.melbourne.sgi.com:/isms/linux/2.6.x-xfs-melb


Modid:  xfs-linux-melb:xfs-kern:27510a
fs/xfs/dmapi/xfs_dm.c - 1.28 - changed
http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/dmapi/xfs_dm.c.diff?r1=text&tr1=1.28&r2=text&tr2=1.27&f=h
        - pv 956783, rv donaldd - Check user buffer address in 
dm_getall_dmattr() for EFAULT error

Re: TAKE 956783 - xfs_dm_getall_dmattr() doesn't check if the user buffer is at valid address

[Christoph Hellwig]

On Tue, Nov 21, 2006 at 05:21:12PM +1100, Vlad Apostolov wrote:
> No EFAULT error when dm_getall_dmattr() called with an invalid user 
> buffer address.

This fix is broken.  access_ok is not enough to verify the buffer,
it just does very few static check (basically the address space limit)

You need to use copy_{from,to}_user to access user pointers.  I had
an untested patch to fix this at my good old SGI time, but Dean wanted
to review and test it a lot more.  I'll try to dig up that patch if you care.

Re: TAKE 956783 - xfs_dm_getall_dmattr() doesn't check if the user buffer is at valid address

[Vlad Apostolov]

Christoph Hellwig wrote:
> On Tue, Nov 21, 2006 at 05:21:12PM +1100, Vlad Apostolov wrote:
>   
>> No EFAULT error when dm_getall_dmattr() called with an invalid user 
>> buffer address.
>>     
>
> This fix is broken.  access_ok is not enough to verify the buffer,
> it just does very few static check (basically the address space limit)
>
> You need to use copy_{from,to}_user to access user pointers.  I had
> an untested patch to fix this at my good old SGI time, but Dean wanted
> to review and test it a lot more.  I'll try to dig up that patch if you care.
>   
The fix is actually fine as it gives an early indication (even not complete)
that the user pointer is bad. There is another problem you are pointing at and 
it is the userspace pointer dereference later on without using copy_to_user(). 
If you have any patch fixing this problem it would be great.

Thanks and regards,
Vlad

Re: TAKE 956783 - xfs_dm_getall_dmattr() doesn't check if the user buffer is at valid address

[Christoph Hellwig]

On Tue, Nov 28, 2006 at 11:00:26AM +1100, Vlad Apostolov wrote:
> The fix is actually fine as it gives an early indication (even not complete)
> that the user pointer is bad. There is another problem you are pointing at 
> and it is the userspace pointer dereference later on without using 
> copy_to_user(). If you have any patch fixing this problem it would be great.

Unfortunately I haven't found my patch, I'm sorry.  I have on the other
hand found various old trivial XFS patches of mine that I'll submit.