[PATCH] sendfile compat functions on x86_64 and ia64

[PATCH] sendfile compat functions on x86_64 and ia64

[Alexey Toptygin]

Hi,

I'm a kernel noob, so I apologise in advance if I completely misunderstood 
something. In arch/x86_64/ia32/sys_ia32.c there is this code:

sys32_sendfile(int out_fd, int in_fd, compat_off_t __user *offset, s32 count)
[snip]
	ret = sys_sendfile(out_fd, in_fd, offset ? &of : NULL, count);

However on ia32, count (a size_t) is u32. I think this is taking the u32 
value from the 32 bit userland, sign-extending it to 64 bits, then giving 
it to sys_sendfile in a u64. So, a count >= 1<<31 passed from the 32 bit 
app will become a count >= ((1<<33)-1)<<31 given to sys_sendfile.

Now, I don't think this actually hurts anything, because sys_sendfile 
passes a max of ((1<<31)-1) to do_sendfile, plus rw_verify_area will 
reject values that are negative when cast to ssize_t; but, this is 
certainly confusing.

Perhaps that s32 should be changed to a compat_size_t? ISTM that's 
what compat_size_t is for. And if so, the equivalent function in 
arch/ia64/ia32/sys_ia32.c:

sys32_sendfile (int out_fd, int in_fd, int __user *offset, unsigned int count)

should probably be changed as well? In case I'm not completely wrong, 
below is a patch. Please CC: me, I'm not on lkml.

Signed-off-by: Alexey Toptygin <alexeyt@freeshell.org>

diff -urpN linux-source-2.6.16/arch/ia64/ia32/sys_ia32.c linux-source-2.6.16-mine/arch/ia64/ia32/sys_ia32.c
--- linux-source-2.6.16/arch/ia64/ia32/sys_ia32.c	2006-03-20 00:53:29.000000000 -0500
+++ linux-source-2.6.16-mine/arch/ia64/ia32/sys_ia32.c	2006-05-04 20:20:44.000000000 -0400
@@ -2306,7 +2306,8 @@ sys32_pwrite (unsigned int fd, void __us
 }
 
 asmlinkage long
-sys32_sendfile (int out_fd, int in_fd, int __user *offset, unsigned int count)
+sys32_sendfile (int out_fd, int in_fd, compat_off_t __user *offset,
+							compat_size_t count)
 {
 	mm_segment_t old_fs = get_fs();
 	long ret;
diff -urpN linux-source-2.6.16/arch/x86_64/ia32/sys_ia32.c linux-source-2.6.16-mine/arch/x86_64/ia32/sys_ia32.c
--- linux-source-2.6.16/arch/x86_64/ia32/sys_ia32.c	2006-03-20 00:53:29.000000000 -0500
+++ linux-source-2.6.16-mine/arch/x86_64/ia32/sys_ia32.c	2006-05-04 20:19:35.000000000 -0400
@@ -760,7 +760,8 @@ sys32_personality(unsigned long personal
 }
 
 asmlinkage long
-sys32_sendfile(int out_fd, int in_fd, compat_off_t __user *offset, s32 count)
+sys32_sendfile(int out_fd, int in_fd, compat_off_t __user *offset,
+							compat_size_t count)
 {
 	mm_segment_t old_fs = get_fs();
 	int ret;

Re: [PATCH] sendfile compat functions on x86_64 and ia64

[Andi Kleen]

On Friday 05 May 2006 02:45, Alexey Toptygin wrote:
> 
> Hi,
> 
> I'm a kernel noob, so I apologise in advance if I completely misunderstood 
> something. In arch/x86_64/ia32/sys_ia32.c there is this code:
> 
> sys32_sendfile(int out_fd, int in_fd, compat_off_t __user *offset, s32 count)
> [snip]
> 	ret = sys_sendfile(out_fd, in_fd, offset ? &of : NULL, count);
> 
> However on ia32, count (a size_t) is u32. I think this is taking the u32 
> value from the 32 bit userland, sign-extending it to 64 bits, then giving 
> it to sys_sendfile in a u64. So, a count >= 1<<31 passed from the 32 bit 
> app will become a count >= ((1<<33)-1)<<31 given to sys_sendfile.
> 
> Now, I don't think this actually hurts anything, because sys_sendfile 
> passes a max of ((1<<31)-1) to do_sendfile, plus rw_verify_area will 
> reject values that are negative when cast to ssize_t; but, this is 
> certainly confusing.

With your change there wouldn't be any sign extension and rw_verify_area
couldn't reject negative values them anymore.

I think it would be a wrong change because it would differ from a native 
32bit kernel.

-Andi

Re: [PATCH] sendfile compat functions on x86_64 and ia64

[Alexey Toptygin]

On Fri, 5 May 2006, Andi Kleen wrote:

> With your change there wouldn't be any sign extension and rw_verify_area
> couldn't reject negative values them anymore.
>
> I think it would be a wrong change because it would differ from a native
> 32bit kernel.

No...

On a 32 bit kernel (and on a 64 bit kernel using the native interface), 
count is passed to sendfile as unsigned. rw_verify_area explicitly casts 
to signed before checking for negativeness. The only place anywhere in the 
kernel that count is signed (other than where rw_verify area explicitly 
casts it for one test) is in the declaration of sys32_sendfile in the 
x86_64 compat code. I'm pretty sure it's supposed to be unsigned there 
too, and the current code is a typo.

 			Alexey

Re: [PATCH] sendfile compat functions on x86_64 and ia64

[Andi Kleen]

On Friday 05 May 2006 22:44, Alexey Toptygin wrote:
> On Fri, 5 May 2006, Andi Kleen wrote:
> 
> > With your change there wouldn't be any sign extension and rw_verify_area
> > couldn't reject negative values them anymore.
> >
> > I think it would be a wrong change because it would differ from a native
> > 32bit kernel.
> 
> No...
> 
> On a 32 bit kernel (and on a 64 bit kernel using the native interface), 
> count is passed to sendfile as unsigned. rw_verify_area explicitly casts 
> to signed

To a 64bit signed.

> before checking for negativeness. The only place anywhere in the  
> kernel that count is signed (other than where rw_verify area explicitly 
> casts it for one test) is in the declaration of sys32_sendfile in the 
> x86_64 compat code. I'm pretty sure it's supposed to be unsigned there 
> too, and the current code is a typo.

It's a 32bit signed. 

Somehow the 32bit signed has to become a 64bit signed to be caught
by rw_verify_area(). The only place that can do that is the compat
layer.

-Andi

Re: [PATCH] sendfile compat functions on x86_64 and ia64

[Alexey Toptygin]

On Fri, 5 May 2006, Andi Kleen wrote:

>> On a 32 bit kernel (and on a 64 bit kernel using the native interface),
>> count is passed to sendfile as unsigned. rw_verify_area explicitly casts
>> to signed
>
> To a 64bit signed.
>
>> before checking for negativeness. The only place anywhere in the
>> kernel that count is signed (other than where rw_verify area explicitly
>> casts it for one test) is in the declaration of sys32_sendfile in the
>> x86_64 compat code. I'm pretty sure it's supposed to be unsigned there
>> too, and the current code is a typo.
>
> It's a 32bit signed.
>
> Somehow the 32bit signed has to become a 64bit signed to be caught
> by rw_verify_area(). The only place that can do that is the compat
> layer.

I still think you misunderstand.

According to 32 bit libc, count is an unsigned 32 bit value. This unsigned 
32 bit value is given to x86_64's sys32_sendfile, which _thinks_ it's a 
signed 32 bit value. sys32_sendfile is passing it to sys_sendfile, which 
expects an unsigned 64 bit value, so the compiler sign-extends it to 64 
bits, then gives it to sys_sendfile as unsigned 64 bits. sys_sendfile 
passes it to do_sendfile with the same type, so it goes there unchanged. 
do_sendfile passes it to rw_verify_area with the same type, so it goes 
there unchanged. rw_verify_area then does this:

         if (unlikely((ssize_t) count < 0))
                 goto Einval;

I agree that this test will pass if we change the declaration of count to 
u32 in sys32_sendfile, but it should pass: this test isn't meant to catch 
values that shouldn't be passed by a 32 bit program, it is only protecting 
against math involving count wrapping later on in rw_verify_area. The test 
agains MAX_NON_LFS (passed via max from sys_sendfile) later in do_sendfile 
will still fail and reject values greater than ((1<<31)-1) passed in from 
32 bit libc.

The ia64 compat path declares count to be unsigned, and presumably this is 
working fine. That path is identical to the above, except sign extension 
doesn't happen: a u32 value is just placed in a u64 variable.

As a result, the x86_64 and ia64 compat paths are inconsistent, so I think 
one of them needs to change. Since every other path from userland into a 
sendfile function has an unsigned count (and none of them appear to be 
broken), I think the change needs to be in x86_64 sys32_sendfile. I think 
the sign extension that is going on there now is completely unnecessary 
and confusing; I think it's a happy accident that it didn't break 
anything.

The only thing my patch does other than changing the signedness of count 
in the declaration of x86_64 sys32_sendfile is relabelling the types of 
offset and count to compat_off_t and compat_size_t. The underlying types 
shouldn't change as a result, but I think this way what is going on is 
much clearer: the compat_ types were defined for exactly this scenario of 
64 bit kernel functions getting off_t and size_t values from a 32 bit 
userland, no?

 			Alexey

Re: [PATCH] sendfile compat functions on x86_64 and ia64

[Andi Kleen]

> I agree that this test will pass if we change the declaration of count to 
> u32 in sys32_sendfile,

Again the compat layer is only supposed to be as good as a native 32bit
kernel. You try to make it better by allowing values that a 32bit
kernel wouldn't allow. But that's not its goal - it just wants to be 
as compatible as possible.

> The only thing my patch does other than changing the signedness of count 
> in the declaration of x86_64 sys32_sendfile is relabelling the types of 
> offset and count to compat_off_t and compat_size_t. The underlying types 
> shouldn't change as a result, but I think this way what is going on is 
> much clearer: the compat_ types were defined for exactly this scenario of 
> 64 bit kernel functions getting off_t and size_t values from a 32 bit 
> userland, no?

The goal isn't to be clear, the goal is to be compatible.

Please stop continue arguing about this - it's useless.

-Andi

Re: [PATCH] sendfile compat functions on x86_64 and ia64

[Alexey Toptygin]

On Sat, 6 May 2006, Andi Kleen wrote:

>> I agree that this test will pass if we change the declaration of count to
>> u32 in sys32_sendfile,
>
> Again the compat layer is only supposed to be as good as a native 32bit
> kernel. You try to make it better by allowing values that a 32bit
> kernel wouldn't allow. But that's not its goal - it just wants to be
> as compatible as possible.

Absolutely not, this is not what I'm trying to do at all! My change will 
not make it accept values that the native 32 bit kernel won't accept - I 
had a very detailed description of why in my last mail.

> The goal isn't to be clear, the goal is to be compatible.
>
> Please stop continue arguing about this - it's useless.

Fine, you're the maintainer.

 			Alexey