| Message ID | 5D24AD2E.8080102@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Could info leak in preserve_iwmmxt_context() ? | expand |
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,
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
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
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
--- 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 { /*
Hi, Julien In this commit 73839798af7e ("ARM: 8790/1: signal: always use __copy_to_user to save iwmmxt context"): * 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