[PATCH] compat: fix use of an uninitialized variable in compat_sys_io_setup()

[PATCH] compat: fix use of an uninitialized variable in compat_sys_io_setup()

[Namhyung Kim]

The upper bytes of ctx64 might contain garbages because it was
set by get_user() which copied only lower 4 bytes as its second
argument points to. Since sys_io_setup() requires its argumet
is properly initialized to 0 we should set it explicitly.

On x86, this was not a problem since its implementation of
get_user() does a C assignment so that it can fill upper bytes
with 0's. But other archs that use __get_user_asm() or something
might have a problem.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Jeff Moyer <jmoyer@redhat.com>
---
 fs/compat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 4376e07febbb..b074e9f79148 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -526,7 +526,7 @@ asmlinkage long
 compat_sys_io_setup(unsigned nr_reqs, u32 __user *ctx32p)
 {
 	long ret;
-	aio_context_t ctx64;
+	aio_context_t ctx64 = 0;
 
 	mm_segment_t oldfs = get_fs();
 	if (unlikely(get_user(ctx64, ctx32p)))
-- 
1.7.3.4.600.g982838b0

Re: [PATCH] compat: fix use of an uninitialized variable in compat_sys_io_setup()

[Andrew Morton]

On Fri, 24 Dec 2010 18:16:26 +0900
Namhyung Kim <namhyung@gmail.com> wrote:

> The upper bytes of ctx64 might contain garbages because it was
> set by get_user() which copied only lower 4 bytes as its second
> argument points to. Since sys_io_setup() requires its argumet
> is properly initialized to 0 we should set it explicitly.
> 
> On x86, this was not a problem since its implementation of
> get_user() does a C assignment so that it can fill upper bytes
> with 0's. But other archs that use __get_user_asm() or something
> might have a problem.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> ---
>  fs/compat.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/compat.c b/fs/compat.c
> index 4376e07febbb..b074e9f79148 100644
> --- a/fs/compat.c
> +++ b/fs/compat.c
> @@ -526,7 +526,7 @@ asmlinkage long
>  compat_sys_io_setup(unsigned nr_reqs, u32 __user *ctx32p)
>  {
>  	long ret;
> -	aio_context_t ctx64;
> +	aio_context_t ctx64 = 0;
>  
>  	mm_segment_t oldfs = get_fs();
>  	if (unlikely(get_user(ctx64, ctx32p)))

Well.  What _should_ a get_user(some_u64, some_u32*) do to `some_u64'?

I don't recall it coming up before but I'd say that the sane, expected
and certainly *safe* behaviour would be for the implementation to zero
out the upper 32 bits of `some_u64'.

If that's the rule then those architectures need fixing.  Did you have
any architectures in mind?

Re: [PATCH] compat: fix use of an uninitialized variable in compat_sys_io_setup()

[Linus Torvalds]

On Mon, Jan 3, 2011 at 1:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 24 Dec 2010 18:16:26 +0900
> Namhyung Kim <namhyung@gmail.com> wrote:
>
>> The upper bytes of ctx64 might contain garbages because it was
>> set by get_user() which copied only lower 4 bytes as its second
>> argument points to. Since sys_io_setup() requires its argumet
>> is properly initialized to 0 we should set it explicitly.
>>
>> On x86, this was not a problem since its implementation of
>> get_user() does a C assignment so that it can fill upper bytes
>> with 0's. But other archs that use __get_user_asm() or something
>> might have a problem.
>>
>> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
>> Cc: Jeff Moyer <jmoyer@redhat.com>
>> ---
>>  fs/compat.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/compat.c b/fs/compat.c
>> index 4376e07febbb..b074e9f79148 100644
>> --- a/fs/compat.c
>> +++ b/fs/compat.c
>> @@ -526,7 +526,7 @@ asmlinkage long
>>  compat_sys_io_setup(unsigned nr_reqs, u32 __user *ctx32p)
>>  {
>>       long ret;
>> -     aio_context_t ctx64;
>> +     aio_context_t ctx64 = 0;
>>
>>       mm_segment_t oldfs = get_fs();
>>       if (unlikely(get_user(ctx64, ctx32p)))
>
> Well.  What _should_ a get_user(some_u64, some_u32*) do to `some_u64'?

We do "get_user(int,char *)" etc all the time. It's expected to work,
and do the proper zero- (or sign-) extension.

The x86 code does:

  #define get_user(x, ptr)
           ...
           (x) = (__typeof__(*(ptr)))__val_gu;

and that's the _only_ correct thing to do. That assignment, with the
proper type expansion, is absolutely vital. And there's no way you can
do that with any asm constructs, exactly because of the whole
sign/zero extension requirement.

If somebody only sets the low 32 bits of the result, that's simply a
bug. That's a seriously buggered uaccess.h.

So NAK on the patch, and please point to what architecture has this problem.

                            Linus