Could info leak in preserve_iwmmxt_context() ?

Could info leak in preserve_iwmmxt_context() ?

[Yang Yingliang]

Hi, Julien

In this commit 73839798af7e ("ARM: 8790/1: signal: always use 
__copy_to_user to save iwmmxt context"):

--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct 
iwmmxt_sigframe __user *frame)
          kframe->magic = IWMMXT_MAGIC;
          kframe->size = IWMMXT_STORAGE_SIZE;
          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
-
-        err = __copy_to_user(frame, kframe, sizeof(*frame));
      } else {
          /*
           * For bug-compatibility with older kernels, some space
@@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct 
iwmmxt_sigframe __user *frame)
           * Set the magic and size appropriately so that properly
           * written userspace can skip it reliably:
           */
-        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
-        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
+        *kframe = (struct iwmmxt_sigframe) {
+            .magic = DUMMY_MAGIC,
+            .size  = IWMMXT_STORAGE_SIZE,
+        };

The storage member of kframe is uninitialized, it seems will lead a info 
leak to userspace ?

In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it 
has no specific behavior
to define the uninitialized member.

Please correct me if I am wrong.

      }

+    err = __copy_to_user(frame, kframe, sizeof(*kframe));
+

Thanks,
Yang


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could info leak in preserve_iwmmxt_context() ?

[Julien Thierry]

Hi Yang,

On 09/07/2019 16:05, Yang Yingliang wrote:
> Hi, Julien
> 
> In this commit 73839798af7e ("ARM: 8790/1: signal: always use
> __copy_to_user to save iwmmxt context"):
> 
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
> iwmmxt_sigframe __user *frame)
>          kframe->magic = IWMMXT_MAGIC;
>          kframe->size = IWMMXT_STORAGE_SIZE;
>          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
> -
> -        err = __copy_to_user(frame, kframe, sizeof(*frame));
>      } else {
>          /*
>           * For bug-compatibility with older kernels, some space
> @@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
> iwmmxt_sigframe __user *frame)
>           * Set the magic and size appropriately so that properly
>           * written userspace can skip it reliably:
>           */
> -        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
> -        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
> +        *kframe = (struct iwmmxt_sigframe) {
> +            .magic = DUMMY_MAGIC,
> +            .size  = IWMMXT_STORAGE_SIZE,
> +        };
> 
> The storage member of kframe is uninitialized, it seems will lead a info
> leak to userspace ?
> 
> In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it
> has no specific behavior
> to define the uninitialized member.
> 
> Please correct me if I am wrong.
> 

My understanding is that when using a compound initializer (either at
variable declaration or by assigning a compound literal like in this
case), the unspecified members get initialized to 0.

In the GNU-C section you mentioned [1] , there is an example:

    You can also initialize fewer than all of a structure variable’s
    members:

    struct pointy
      {
        int x, y;
        char *p;
      };
    struct pointy first_pointy = { 5 };

    Here, x is initialized with 5, y is initialized with 0, and p is
    initialized with NULL. The rule here is that y and p are initialized
    just as they would be if they were static variables.


So even when the manual refers to not initializing members, I think it
just means that they are not explicitly initialized, i.e. by the
developer. All the members of the structure still gets initialized to
known values when doing an assignment to the whole structure.

One thing that Russell did mention was that initialization of padding
bytes (that aren't part of a structure member but still within the
structure's space) is unspecified. But in the case of iwmmxt_sigframe
there is no padding.

[1]
https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members

Cheers,

-- 
Julien Thierry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could info leak in preserve_iwmmxt_context() ?

[Julien Thierry]

On 09/07/2019 16:30, Julien Thierry wrote:
> Hi Yang,
> 
> On 09/07/2019 16:05, Yang Yingliang wrote:
>> Hi, Julien
>>
>> In this commit 73839798af7e ("ARM: 8790/1: signal: always use
>> __copy_to_user to save iwmmxt context"):
>>
>> --- a/arch/arm/kernel/signal.c
>> +++ b/arch/arm/kernel/signal.c
>> @@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
>> iwmmxt_sigframe __user *frame)
>>          kframe->magic = IWMMXT_MAGIC;
>>          kframe->size = IWMMXT_STORAGE_SIZE;
>>          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
>> -
>> -        err = __copy_to_user(frame, kframe, sizeof(*frame));
>>      } else {
>>          /*
>>           * For bug-compatibility with older kernels, some space
>> @@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
>> iwmmxt_sigframe __user *frame)
>>           * Set the magic and size appropriately so that properly
>>           * written userspace can skip it reliably:
>>           */
>> -        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
>> -        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
>> +        *kframe = (struct iwmmxt_sigframe) {
>> +            .magic = DUMMY_MAGIC,
>> +            .size  = IWMMXT_STORAGE_SIZE,
>> +        };
>>
>> The storage member of kframe is uninitialized, it seems will lead a info
>> leak to userspace ?
>>
>> In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it
>> has no specific behavior
>> to define the uninitialized member.
>>
>> Please correct me if I am wrong.
>>
> 
> My understanding is that when using a compound initializer (either at
> variable declaration or by assigning a compound literal like in this
> case), the unspecified members get initialized to 0.
> 

Also, to back that claim a bit more, when using designated initializers[1]:

"Omitted fields are implicitly initialized the same as for objects that
have static storage duration."

[1]
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Inits

-- 
Julien Thierry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could info leak in preserve_iwmmxt_context() ?

[Dave Martin]

On Tue, Jul 09, 2019 at 04:34:57PM +0100, Julien Thierry wrote:
> 
> 
> On 09/07/2019 16:30, Julien Thierry wrote:
> > Hi Yang,
> > 
> > On 09/07/2019 16:05, Yang Yingliang wrote:
> >> Hi, Julien
> >>
> >> In this commit 73839798af7e ("ARM: 8790/1: signal: always use
> >> __copy_to_user to save iwmmxt context"):
> >>
> >> --- a/arch/arm/kernel/signal.c
> >> +++ b/arch/arm/kernel/signal.c
> >> @@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
> >> iwmmxt_sigframe __user *frame)
> >>          kframe->magic = IWMMXT_MAGIC;
> >>          kframe->size = IWMMXT_STORAGE_SIZE;
> >>          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
> >> -
> >> -        err = __copy_to_user(frame, kframe, sizeof(*frame));
> >>      } else {
> >>          /*
> >>           * For bug-compatibility with older kernels, some space
> >> @@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
> >> iwmmxt_sigframe __user *frame)
> >>           * Set the magic and size appropriately so that properly
> >>           * written userspace can skip it reliably:
> >>           */
> >> -        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
> >> -        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
> >> +        *kframe = (struct iwmmxt_sigframe) {
> >> +            .magic = DUMMY_MAGIC,
> >> +            .size  = IWMMXT_STORAGE_SIZE,
> >> +        };
> >>
> >> The storage member of kframe is uninitialized, it seems will lead a info
> >> leak to userspace ?
> >>
> >> In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it
> >> has no specific behavior
> >> to define the uninitialized member.
> >>
> >> Please correct me if I am wrong.
> >>
> > 
> > My understanding is that when using a compound initializer (either at
> > variable declaration or by assigning a compound literal like in this
> > case), the unspecified members get initialized to 0.
> > 
> 
> Also, to back that claim a bit more, when using designated initializers[1]:
> 
> "Omitted fields are implicitly initialized the same as for objects that
> have static storage duration."

We also rely on this elsewhere IIUC.

I don't think this guarantee extends to padding though, so watch out
for that.

For this case, it looks like struct iwmmxt_sigframe is padding-free
though.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could info leak in preserve_iwmmxt_context() ?

[Hanjun Guo]

Hi Julien, Dave,

On 2019/7/10 0:47, Dave Martin wrote:
> On Tue, Jul 09, 2019 at 04:34:57PM +0100, Julien Thierry wrote:
>>
>>
>> On 09/07/2019 16:30, Julien Thierry wrote:
>>> Hi Yang,
>>>
>>> On 09/07/2019 16:05, Yang Yingliang wrote:
>>>> Hi, Julien
>>>>
>>>> In this commit 73839798af7e ("ARM: 8790/1: signal: always use
>>>> __copy_to_user to save iwmmxt context"):
>>>>
>>>> --- a/arch/arm/kernel/signal.c
>>>> +++ b/arch/arm/kernel/signal.c
>>>> @@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct
>>>> iwmmxt_sigframe __user *frame)
>>>>          kframe->magic = IWMMXT_MAGIC;
>>>>          kframe->size = IWMMXT_STORAGE_SIZE;
>>>>          iwmmxt_task_copy(current_thread_info(), &kframe->storage);
>>>> -
>>>> -        err = __copy_to_user(frame, kframe, sizeof(*frame));
>>>>      } else {
>>>>          /*
>>>>           * For bug-compatibility with older kernels, some space
>>>> @@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct
>>>> iwmmxt_sigframe __user *frame)
>>>>           * Set the magic and size appropriately so that properly
>>>>           * written userspace can skip it reliably:
>>>>           */
>>>> -        __put_user_error(DUMMY_MAGIC, &frame->magic, err);
>>>> -        __put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
>>>> +        *kframe = (struct iwmmxt_sigframe) {
>>>> +            .magic = DUMMY_MAGIC,
>>>> +            .size  = IWMMXT_STORAGE_SIZE,
>>>> +        };
>>>>
>>>> The storage member of kframe is uninitialized, it seems will lead a info
>>>> leak to userspace ?
>>>>
>>>> In section 2.4.2.3 Initializing Structure Members of gnu-c-manual, it
>>>> has no specific behavior
>>>> to define the uninitialized member.
>>>>
>>>> Please correct me if I am wrong.
>>>>
>>>
>>> My understanding is that when using a compound initializer (either at
>>> variable declaration or by assigning a compound literal like in this
>>> case), the unspecified members get initialized to 0.
>>>
>>
>> Also, to back that claim a bit more, when using designated initializers[1]:
>>
>> "Omitted fields are implicitly initialized the same as for objects that
>> have static storage duration."
> 
> We also rely on this elsewhere IIUC.
> 
> I don't think this guarantee extends to padding though, so watch out
> for that.
> 
> For this case, it looks like struct iwmmxt_sigframe is padding-free
> though.

Thank you for the clarify, that's crystal clear for us now.

Thanks
Hanjun


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel