Re: Could info leak in preserve_iwmmxt_context() ?

[Hanjun Guo <guohanjun@huawei.com>]

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