rds_notify_queue_get() is potentially copying uninitialized kernel stack
memory to userspace since the compiler may leave a 4-byte hole at the end
of `cmsg`.
In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
unfortunately does not always initialize that 4-byte hole. Fix it by using
memset() instead.
Cc: [email protected]
Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
Fixes: bdbe6fbc6a2f ("RDS: recv.c")
Suggested-by: Dan Carpenter <[email protected]>
Signed-off-by: Peilin Ye <[email protected]>
---
Note: the "real" copy_to_user() happens in put_cmsg(), where
`cmlen - sizeof(*cm)` equals to `sizeof(cmsg)`.
Reference: https://lwn.net/Articles/417989/
$ pahole -C "rds_rdma_notify" net/rds/recv.o
struct rds_rdma_notify {
__u64 user_token; /* 0 8 */
__s32 status; /* 8 4 */
/* size: 16, cachelines: 1, members: 2 */
/* padding: 4 */
/* last cacheline: 16 bytes */
};
net/rds/recv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/rds/recv.c b/net/rds/recv.c
index c8404971d5ab..aba4afe4dfed 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -450,12 +450,13 @@ static int rds_still_queued(struct rds_sock *rs, struct rds_incoming *inc,
int rds_notify_queue_get(struct rds_sock *rs, struct msghdr *msghdr)
{
struct rds_notifier *notifier;
- struct rds_rdma_notify cmsg = { 0 }; /* fill holes with zero */
+ struct rds_rdma_notify cmsg;
unsigned int count = 0, max_messages = ~0U;
unsigned long flags;
LIST_HEAD(copy);
int err = 0;
+ memset(&cmsg, 0, sizeof(cmsg)); /* fill holes with zero */
/* put_cmsg copies to user space and thus may sleep. We can't do this
* with rs_lock held, so first grab as many notifications as we can stuff
--
2.25.1
On 7/30/20 12:20 PM, Peilin Ye wrote:
> rds_notify_queue_get() is potentially copying uninitialized kernel stack
> memory to userspace since the compiler may leave a 4-byte hole at the end
> of `cmsg`.
>
> In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> unfortunately does not always initialize that 4-byte hole. Fix it by using
> memset() instead.
>
> Cc: [email protected]
> Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
> Fixes: bdbe6fbc6a2f ("RDS: recv.c")
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> Note: the "real" copy_to_user() happens in put_cmsg(), where
> `cmlen - sizeof(*cm)` equals to `sizeof(cmsg)`.
>
> Reference: https://lwn.net/Articles/417989/
>
> $ pahole -C "rds_rdma_notify" net/rds/recv.o
> struct rds_rdma_notify {
> __u64 user_token; /* 0 8 */
> __s32 status; /* 8 4 */
>
> /* size: 16, cachelines: 1, members: 2 */
> /* padding: 4 */
> /* last cacheline: 16 bytes */
> };
>
> net/rds/recv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
Looks good.
FWIW,
Acked-by: Santosh Shilimkar <[email protected]>
On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> rds_notify_queue_get() is potentially copying uninitialized kernel stack
> memory to userspace since the compiler may leave a 4-byte hole at the end
> of `cmsg`.
>
> In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> unfortunately does not always initialize that 4-byte hole. Fix it by using
> memset() instead.
Of course, this is the difference between "{ 0 }" and "{}" initializations.
>
> Cc: [email protected]
> Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
> Fixes: bdbe6fbc6a2f ("RDS: recv.c")
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
> ---
> Note: the "real" copy_to_user() happens in put_cmsg(), where
> `cmlen - sizeof(*cm)` equals to `sizeof(cmsg)`.
>
> Reference: https://lwn.net/Articles/417989/
>
> $ pahole -C "rds_rdma_notify" net/rds/recv.o
> struct rds_rdma_notify {
> __u64 user_token; /* 0 8 */
> __s32 status; /* 8 4 */
>
> /* size: 16, cachelines: 1, members: 2 */
> /* padding: 4 */
> /* last cacheline: 16 bytes */
> };
>
> net/rds/recv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index c8404971d5ab..aba4afe4dfed 100644
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c
> @@ -450,12 +450,13 @@ static int rds_still_queued(struct rds_sock *rs, struct rds_incoming *inc,
> int rds_notify_queue_get(struct rds_sock *rs, struct msghdr *msghdr)
> {
> struct rds_notifier *notifier;
> - struct rds_rdma_notify cmsg = { 0 }; /* fill holes with zero */
> + struct rds_rdma_notify cmsg;
> unsigned int count = 0, max_messages = ~0U;
> unsigned long flags;
> LIST_HEAD(copy);
> int err = 0;
>
> + memset(&cmsg, 0, sizeof(cmsg)); /* fill holes with zero */
It works, but the right solution is to drop 0 from cmsg initialization
and write "struct rds_rdma_notify cmsg = {};" without any memset.
Thanks
On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> > rds_notify_queue_get() is potentially copying uninitialized kernel stack
> > memory to userspace since the compiler may leave a 4-byte hole at the end
> > of `cmsg`.
> >
> > In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> > unfortunately does not always initialize that 4-byte hole. Fix it by using
> > memset() instead.
>
> Of course, this is the difference between "{ 0 }" and "{}" initializations.
Really? Neither will handle structures with holes in it, try it and
see.
thanks,
greg k-h
On Fri, Jul 31, 2020 at 07:33:06AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> > On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> > > rds_notify_queue_get() is potentially copying uninitialized kernel stack
> > > memory to userspace since the compiler may leave a 4-byte hole at the end
> > > of `cmsg`.
> > >
> > > In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> > > unfortunately does not always initialize that 4-byte hole. Fix it by using
> > > memset() instead.
> >
> > Of course, this is the difference between "{ 0 }" and "{}" initializations.
>
> Really? Neither will handle structures with holes in it, try it and
> see.
And if true, where in the C spec does it say that?
thanks,
greg k-h
On Fri, Jul 31, 2020 at 09:29:27AM +0300, Andy Shevchenko wrote:
> On Friday, July 31, 2020, Greg Kroah-Hartman <[email protected]>
> wrote:
>
> > On Fri, Jul 31, 2020 at 07:33:06AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> > > > On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> > > > > rds_notify_queue_get() is potentially copying uninitialized kernel
> > stack
> > > > > memory to userspace since the compiler may leave a 4-byte hole at
> > the end
> > > > > of `cmsg`.
> > > > >
> > > > > In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`,
> > which
> > > > > unfortunately does not always initialize that 4-byte hole. Fix it by
> > using
> > > > > memset() instead.
> > > >
> > > > Of course, this is the difference between "{ 0 }" and "{}"
> > initializations.
> > >
> > > Really? Neither will handle structures with holes in it, try it and
> > > see.
>
>
> {} is a GCC extension, but I never thought it works differently.
Yes, this is GCC extension and kernel relies on them very heavily.
Thanks
>
>
>
> >
> > And if true, where in the C spec does it say that?
> >
> > thanks,
> >
> > greg k-h
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
Sakari, JFYI. I remember during some reviews we have a discussion
about {0} vs {} and surprisingly they are not an equivalent.
On Fri, Jul 31, 2020 at 10:00 AM Leon Romanovsky <[email protected]> wrote:
> On Fri, Jul 31, 2020 at 09:29:27AM +0300, Andy Shevchenko wrote:
> > On Friday, July 31, 2020, Greg Kroah-Hartman <[email protected]>
> > wrote:
> > > On Fri, Jul 31, 2020 at 07:33:06AM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> > > > > On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
...
> > > > > Of course, this is the difference between "{ 0 }" and "{}"
> > > initializations.
> > > >
> > > > Really? Neither will handle structures with holes in it, try it and
> > > > see.
> >
> >
> > {} is a GCC extension, but I never thought it works differently.
>
> Yes, this is GCC extension and kernel relies on them very heavily.
I guess simple people who contribute to the kernel just haven't
realized (yet) that it's an extension and that's why we have plenty of
{} and {0} in the kernel.
> > > And if true, where in the C spec does it say that?
--
With Best Regards,
Andy Shevchenko
On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> > rds_notify_queue_get() is potentially copying uninitialized kernel stack
> > memory to userspace since the compiler may leave a 4-byte hole at the end
> > of `cmsg`.
> >
> > In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> > unfortunately does not always initialize that 4-byte hole. Fix it by using
> > memset() instead.
>
> Of course, this is the difference between "{ 0 }" and "{}" initializations.
>
No, there is no difference. Even struct assignments like:
foo = *bar;
can leave struct holes uninitialized. Depending on the compiler the
assignment can be implemented as a memset() or as a series of struct
member assignments.
regards,
dan carpenter
> On 31 Jul 2020, at 11:59, Dan Carpenter <[email protected]> wrote:
>
> On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
>> On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
>>> rds_notify_queue_get() is potentially copying uninitialized kernel stack
>>> memory to userspace since the compiler may leave a 4-byte hole at the end
>>> of `cmsg`.
>>>
>>> In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
>>> unfortunately does not always initialize that 4-byte hole. Fix it by using
>>> memset() instead.
>>
>> Of course, this is the difference between "{ 0 }" and "{}" initializations.
>>
>
> No, there is no difference. Even struct assignments like:
>
> foo = *bar;
>
> can leave struct holes uninitialized. Depending on the compiler the
> assignment can be implemented as a memset() or as a series of struct
> member assignments.
What about:
struct rds_rdma_notify {
__u64 user_token;
__s32 status;
} __attribute__((packed));
Thxs, Håkon
> regards,
> dan carpenter
>
On Fri, Jul 31, 2020 at 01:14:09PM +0200, H?kon Bugge wrote:
>
>
> > On 31 Jul 2020, at 11:59, Dan Carpenter <[email protected]> wrote:
> >
> > On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> >> On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> >>> rds_notify_queue_get() is potentially copying uninitialized kernel stack
> >>> memory to userspace since the compiler may leave a 4-byte hole at the end
> >>> of `cmsg`.
> >>>
> >>> In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> >>> unfortunately does not always initialize that 4-byte hole. Fix it by using
> >>> memset() instead.
> >>
> >> Of course, this is the difference between "{ 0 }" and "{}" initializations.
> >>
> >
> > No, there is no difference. Even struct assignments like:
> >
> > foo = *bar;
> >
> > can leave struct holes uninitialized. Depending on the compiler the
> > assignment can be implemented as a memset() or as a series of struct
> > member assignments.
>
> What about:
>
> struct rds_rdma_notify {
> __u64 user_token;
> __s32 status;
> } __attribute__((packed));
Why is this still a discussion at all?
Try it and see, run pahole and see if there are holes in this structure
(odds are no), you don't need us to say what is happening here...
thanks,
greg k-h
> On 31 Jul 2020, at 13:59, Greg Kroah-Hartman <[email protected]> wrote:
>
> On Fri, Jul 31, 2020 at 01:14:09PM +0200, Håkon Bugge wrote:
>>
>>
>>> On 31 Jul 2020, at 11:59, Dan Carpenter <[email protected]> wrote:
>>>
>>> On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
>>>> On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
>>>>> rds_notify_queue_get() is potentially copying uninitialized kernel stack
>>>>> memory to userspace since the compiler may leave a 4-byte hole at the end
>>>>> of `cmsg`.
>>>>>
>>>>> In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
>>>>> unfortunately does not always initialize that 4-byte hole. Fix it by using
>>>>> memset() instead.
>>>>
>>>> Of course, this is the difference between "{ 0 }" and "{}" initializations.
>>>>
>>>
>>> No, there is no difference. Even struct assignments like:
>>>
>>> foo = *bar;
>>>
>>> can leave struct holes uninitialized. Depending on the compiler the
>>> assignment can be implemented as a memset() or as a series of struct
>>> member assignments.
>>
>> What about:
>>
>> struct rds_rdma_notify {
>> __u64 user_token;
>> __s32 status;
>> } __attribute__((packed));
>
> Why is this still a discussion at all?
>
> Try it and see, run pahole and see if there are holes in this structure
> (odds are no), you don't need us to say what is happening here...
An older posting had this:
$ pahole -C "rds_rdma_notify" net/rds/recv.o
struct rds_rdma_notify {
__u64 user_token; /* 0 8 */
__s32 status; /* 8 4 */
/* size: 16, cachelines: 1, members: 2 */
/* padding: 4 */
/* last cacheline: 16 bytes */
};
Thxs, Håkon
On Fri, Jul 31, 2020 at 07:33:33AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 31, 2020 at 07:33:06AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> > > On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> > > > rds_notify_queue_get() is potentially copying uninitialized kernel stack
> > > > memory to userspace since the compiler may leave a 4-byte hole at the end
> > > > of `cmsg`.
> > > >
> > > > In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> > > > unfortunately does not always initialize that 4-byte hole. Fix it by using
> > > > memset() instead.
> > >
> > > Of course, this is the difference between "{ 0 }" and "{}" initializations.
> >
> > Really? Neither will handle structures with holes in it, try it and
> > see.
>
> And if true, where in the C spec does it say that?
The spec was updated in C11 to require zero'ing padding when doing
partial initialization of aggregates (eg = {})
"""if it is an aggregate, every member is initialized (recursively)
according to these rules, and any padding is initialized to zero
bits;"""
The difference between {0} and the {} extension is only that {}
reliably triggers partial initialization for all kinds of aggregates,
while {0} has a number of edge cases where it can fail to compile.
IIRC gcc has cleared the padding during aggregate initialization for a
long time. Considering we have thousands of aggregate initializers it
seems likely to me Linux also requires a compiler with this C11
behavior to operate correctly.
Does this patch actually fix anything? My compiler generates identical
assembly code in either case.
Jason
On Fri, Jul 31, 2020 at 11:04:52AM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 31, 2020 at 07:33:33AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Jul 31, 2020 at 07:33:06AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jul 31, 2020 at 07:53:01AM +0300, Leon Romanovsky wrote:
> > > > On Thu, Jul 30, 2020 at 03:20:26PM -0400, Peilin Ye wrote:
> > > > > rds_notify_queue_get() is potentially copying uninitialized kernel stack
> > > > > memory to userspace since the compiler may leave a 4-byte hole at the end
> > > > > of `cmsg`.
> > > > >
> > > > > In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> > > > > unfortunately does not always initialize that 4-byte hole. Fix it by using
> > > > > memset() instead.
> > > >
> > > > Of course, this is the difference between "{ 0 }" and "{}" initializations.
> > >
> > > Really? Neither will handle structures with holes in it, try it and
> > > see.
> >
> > And if true, where in the C spec does it say that?
>
> The spec was updated in C11 to require zero'ing padding when doing
> partial initialization of aggregates (eg = {})
>
> """if it is an aggregate, every member is initialized (recursively)
> according to these rules, and any padding is initialized to zero
> bits;"""
But then why does the compilers not do this?
> The difference between {0} and the {} extension is only that {}
> reliably triggers partial initialization for all kinds of aggregates,
> while {0} has a number of edge cases where it can fail to compile.
>
> IIRC gcc has cleared the padding during aggregate initialization for a
> long time.
Huh? Last we checked a few months ago, no, it did not do that.
> Considering we have thousands of aggregate initializers it
> seems likely to me Linux also requires a compiler with this C11
> behavior to operate correctly.
Note that this is not an "operate correctly" thing, it is a "zero out
stale data in structure paddings so that data will not leak to
userspace" thing.
> Does this patch actually fix anything? My compiler generates identical
> assembly code in either case.
What compiler version?
thanks,
greg k-h
On Fri, Jul 31, 2020 at 04:21:48PM +0200, Greg Kroah-Hartman wrote:
> > The spec was updated in C11 to require zero'ing padding when doing
> > partial initialization of aggregates (eg = {})
> >
> > """if it is an aggregate, every member is initialized (recursively)
> > according to these rules, and any padding is initialized to zero
> > bits;"""
>
> But then why does the compilers not do this?
Do you have an example?
> > Considering we have thousands of aggregate initializers it
> > seems likely to me Linux also requires a compiler with this C11
> > behavior to operate correctly.
>
> Note that this is not an "operate correctly" thing, it is a "zero out
> stale data in structure paddings so that data will not leak to
> userspace" thing.
Yes, not being insecure is "operate correctly", IMHO :)
> > Does this patch actually fix anything? My compiler generates identical
> > assembly code in either case.
>
> What compiler version?
I tried clang 10 and gcc 9.3 for x86-64.
#include <string.h>
void test(void *out)
{
struct rds_rdma_notify {
unsigned long user_token;
unsigned int status;
} foo = {};
memcpy(out, &foo, sizeof(foo));
}
$ gcc -mno-sse2 -O2 -Wall -std=c99 t.c -S
test:
endbr64
movq $0, (%rdi)
movq $0, 8(%rdi)
ret
Just did this same test with gcc 4.4 and it also gave the same output..
Made it more complex with this:
struct rds_rdma_notify {
unsigned long user_token;
unsigned char status;
unsigned long user_token1;
unsigned char status1;
unsigned long user_token2;
unsigned char status2;
unsigned long user_token3;
unsigned char status3;
unsigned long user_token4;
unsigned char status4;
} foo;
And still got the same assembly vs memset on gcc 4.4.
I tried for a bit and didn't find a way to get even old gcc 4.4 to not
initialize the holes.
Jason
On Fri, Jul 31, 2020 at 11:36:04AM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 31, 2020 at 04:21:48PM +0200, Greg Kroah-Hartman wrote:
>
> > > The spec was updated in C11 to require zero'ing padding when doing
> > > partial initialization of aggregates (eg = {})
> > >
> > > """if it is an aggregate, every member is initialized (recursively)
> > > according to these rules, and any padding is initialized to zero
> > > bits;"""
> >
> > But then why does the compilers not do this?
>
> Do you have an example?
At the moment, no, but we have had them in the past due to security
issues we have had to fix for this.
> > > Considering we have thousands of aggregate initializers it
> > > seems likely to me Linux also requires a compiler with this C11
> > > behavior to operate correctly.
> >
> > Note that this is not an "operate correctly" thing, it is a "zero out
> > stale data in structure paddings so that data will not leak to
> > userspace" thing.
>
> Yes, not being insecure is "operate correctly", IMHO :)
>
> > > Does this patch actually fix anything? My compiler generates identical
> > > assembly code in either case.
> >
> > What compiler version?
>
> I tried clang 10 and gcc 9.3 for x86-64.
>
> #include <string.h>
>
> void test(void *out)
> {
> struct rds_rdma_notify {
> unsigned long user_token;
> unsigned int status;
> } foo = {};
> memcpy(out, &foo, sizeof(foo));
> }
>
> $ gcc -mno-sse2 -O2 -Wall -std=c99 t.c -S
>
> test:
> endbr64
> movq $0, (%rdi)
> movq $0, 8(%rdi)
> ret
>
> Just did this same test with gcc 4.4 and it also gave the same output..
>
> Made it more complex with this:
>
> struct rds_rdma_notify {
> unsigned long user_token;
> unsigned char status;
> unsigned long user_token1;
> unsigned char status1;
> unsigned long user_token2;
> unsigned char status2;
> unsigned long user_token3;
> unsigned char status3;
> unsigned long user_token4;
> unsigned char status4;
> } foo;
>
> And still got the same assembly vs memset on gcc 4.4.
>
> I tried for a bit and didn't find a way to get even old gcc 4.4 to not
> initialize the holes.
Odd, so it is just the "= {0};" that does not zero out the holes?
thanks,
greg k-h
On Fri, Jul 31, 2020 at 07:19:24PM +0200, Greg Kroah-Hartman wrote:
> > I tried for a bit and didn't find a way to get even old gcc 4.4 to not
> > initialize the holes.
>
> Odd, so it is just the "= {0};" that does not zero out the holes?
Nope, it seems to work fine too. I tried a number of situations and I
could not get the compiler to not zero holes, even back to gcc 4.4
It is not just accidental either, take this:
struct rds_rdma_notify {
unsigned long user_token;
unsigned char status;
unsigned long user_token1 __attribute__((aligned(32)));
} foo = {0};
Which has quite a big hole, clang generates:
movq $0, 56(%rdi)
movq $0, 48(%rdi)
movq $0, 40(%rdi)
movq $0, 32(%rdi)
movq $0, 24(%rdi)
movq $0, 16(%rdi)
movq $0, 8(%rdi)
movq $0, (%rdi)
Deliberate extra instructions to fill both holes. gcc 10 does the
same, older gcc's do create a rep stosq over the whole thing.
Some fiddling with godbolt shows quite a variety of output, but I
didn't see anything that looks like a compiler not filling
padding. Even godbolt's gcc 4.1 filled the padding, which is super old.
In several cases it seems the aggregate initializer produced better
code than memset, in other cases it didn't
Without an actual example where this doesn't work right it is hard to
say anything more..
Jason
From: Peilin Ye <[email protected]>
Date: Thu, 30 Jul 2020 15:20:26 -0400
> rds_notify_queue_get() is potentially copying uninitialized kernel stack
> memory to userspace since the compiler may leave a 4-byte hole at the end
> of `cmsg`.
>
> In 2016 we tried to fix this issue by doing `= { 0 };` on `cmsg`, which
> unfortunately does not always initialize that 4-byte hole. Fix it by using
> memset() instead.
>
> Cc: [email protected]
> Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
> Fixes: bdbe6fbc6a2f ("RDS: recv.c")
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Peilin Ye <[email protected]>
Applied and queued up for -stable, thanks.
I saw a suggestion to use __packed but that breaks UAPI and is definitely
not an option to solve this problem.
On Fri, Jul 31, 2020 at 07:19:24PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 31, 2020 at 11:36:04AM -0300, Jason Gunthorpe wrote:
> > On Fri, Jul 31, 2020 at 04:21:48PM +0200, Greg Kroah-Hartman wrote:
> >
> > > > The spec was updated in C11 to require zero'ing padding when doing
> > > > partial initialization of aggregates (eg = {})
> > > >
> > > > """if it is an aggregate, every member is initialized (recursively)
> > > > according to these rules, and any padding is initialized to zero
> > > > bits;"""
> > >
> > > But then why does the compilers not do this?
> >
> > Do you have an example?
>
> At the moment, no, but we have had them in the past due to security
> issues we have had to fix for this.
Is it still relevant after bump of required GCC version to build kernel?
I afraid that without solid example such changes will start to be
treated with cargo cult.
Jason,
I'm using {} instead of {0} because of this GCC bug.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
Thanks
On Fri, Jul 31, 2020 at 03:27:12PM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 31, 2020 at 07:19:24PM +0200, Greg Kroah-Hartman wrote:
>
> > > I tried for a bit and didn't find a way to get even old gcc 4.4 to not
> > > initialize the holes.
> >
> > Odd, so it is just the "= {0};" that does not zero out the holes?
>
> Nope, it seems to work fine too. I tried a number of situations and I
> could not get the compiler to not zero holes, even back to gcc 4.4
>
> It is not just accidental either, take this:
>
> struct rds_rdma_notify {
> unsigned long user_token;
> unsigned char status;
> unsigned long user_token1 __attribute__((aligned(32)));
> } foo = {0};
>
> Which has quite a big hole, clang generates:
>
> movq $0, 56(%rdi)
> movq $0, 48(%rdi)
> movq $0, 40(%rdi)
> movq $0, 32(%rdi)
> movq $0, 24(%rdi)
> movq $0, 16(%rdi)
> movq $0, 8(%rdi)
> movq $0, (%rdi)
>
> Deliberate extra instructions to fill both holes. gcc 10 does the
> same, older gcc's do create a rep stosq over the whole thing.
>
> Some fiddling with godbolt shows quite a variety of output, but I
> didn't see anything that looks like a compiler not filling
> padding. Even godbolt's gcc 4.1 filled the padding, which is super old.
>
> In several cases it seems the aggregate initializer produced better
> code than memset, in other cases it didn't
>
> Without an actual example where this doesn't work right it is hard to
> say anything more..
Here is the example that set off the recent patches:
https://lkml.org/lkml/2020/7/27/199
Another example is commit 5ff223e86f5a ("net: Zeroing the structure
ethtool_wolinfo in ethtool_get_wol()"). I tested this one with GCC 7.4
at the time and it was a real life bug.
The rest of these patches were based on static analysis from Smatch.
They're all "theoretical" bugs based on the C standard but it's
impossible to know if and when they'll turn into real life bugs.
It's not a super long list of code that's affected because we've known
that the bug was possible for a few years. It was only last year when
I saw that it had become a real life bug.
regards,
dan carpenter
On Sat, Aug 01, 2020 at 11:00:26AM +0300, Dan Carpenter wrote:
> > Without an actual example where this doesn't work right it is hard to
> > say anything more..
>
> Here is the example that set off the recent patches:
>
> https://lkml.org/lkml/2020/7/27/199
Oh, that is something completely different. This thread was talking
about '= {}'.
From a C11 perspective the above link is complete initialization of an
aggregate and does not trigger the rule requiring that padding be
zero'd.
C11 only zeros padding during *partial* initialization of an aggregate.
ie this does not zero padding:
void test(void)
{
extern void copy(const void *ptr, size_t len);
struct rds_rdma_notify {
unsigned long user_token;
unsigned char status __attribute__((aligned(32)));
} foo = {1, 1};
// Padding NOT zeroed
copy(&foo, sizeof(foo));
}
While the addition of a xxx member to make it partial initialization
does zero:
void test(void)
{
extern void copy(const void *ptr, size_t len);
struct rds_rdma_notify {
unsigned long user_token;
unsigned char status __attribute__((aligned(32)));
unsigned long xx;
} foo = {1, 1};
// Padding NOT zeroed
copy(&foo, sizeof(foo));
}
(and godbolt confirms this on a wide range of compilers)
> The rest of these patches were based on static analysis from Smatch.
> They're all "theoretical" bugs based on the C standard but it's
> impossible to know if and when they'll turn into real life bugs.
Any patches replaing '= {}' (and usually '= {0}') with memset are not
fixing anything.
The C11 standard requires zeroing padding in these case. It is just
useless churn and in some cases results in worse codegen.
smatch should only warn about this if the aggregate initialization is
not partial.
Jason
On Sat, Aug 01, 2020 at 08:38:33AM +0300, Leon Romanovsky wrote:
> I'm using {} instead of {0} because of this GCC bug.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
This is why the {} extension exists..
Jason
On Sun, Aug 02, 2020 at 03:23:58PM -0700, Joe Perches wrote:
> On Sun, 2020-08-02 at 19:10 -0300, Jason Gunthorpe wrote:
> > On Sat, Aug 01, 2020 at 08:38:33AM +0300, Leon Romanovsky wrote:
> >
> > > I'm using {} instead of {0} because of this GCC bug.
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> >
> > This is why the {} extension exists..
>
> There is no guarantee that the gcc struct initialization {}
> extension also zeros padding.
We just went over this. Yes there is, C11 requires it.
Jason
On Sun, 2020-08-02 at 19:10 -0300, Jason Gunthorpe wrote:
> On Sat, Aug 01, 2020 at 08:38:33AM +0300, Leon Romanovsky wrote:
>
> > I'm using {} instead of {0} because of this GCC bug.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
>
> This is why the {} extension exists..
There is no guarantee that the gcc struct initialization {}
extension also zeros padding.
On Sun, 2020-08-02 at 19:28 -0300, Jason Gunthorpe wrote:
> On Sun, Aug 02, 2020 at 03:23:58PM -0700, Joe Perches wrote:
> > On Sun, 2020-08-02 at 19:10 -0300, Jason Gunthorpe wrote:
> > > On Sat, Aug 01, 2020 at 08:38:33AM +0300, Leon Romanovsky wrote:
> > >
> > > > I'm using {} instead of {0} because of this GCC bug.
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> > >
> > > This is why the {} extension exists..
> >
> > There is no guarantee that the gcc struct initialization {}
> > extension also zeros padding.
>
> We just went over this. Yes there is, C11 requires it.
c11 is not c90. The kernel uses c90.
On Sun, Aug 02, 2020 at 03:45:40PM -0700, Joe Perches wrote:
> On Sun, 2020-08-02 at 19:28 -0300, Jason Gunthorpe wrote:
> > On Sun, Aug 02, 2020 at 03:23:58PM -0700, Joe Perches wrote:
> > > On Sun, 2020-08-02 at 19:10 -0300, Jason Gunthorpe wrote:
> > > > On Sat, Aug 01, 2020 at 08:38:33AM +0300, Leon Romanovsky wrote:
> > > >
> > > > > I'm using {} instead of {0} because of this GCC bug.
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> > > >
> > > > This is why the {} extension exists..
> > >
> > > There is no guarantee that the gcc struct initialization {}
> > > extension also zeros padding.
> >
> > We just went over this. Yes there is, C11 requires it.
>
> c11 is not c90. The kernel uses c90.
It is not accurate, kernel uses gnu89 dialect, which is C90 with some
C99 features [1]. In our case, we rely on GCC extension {} that doesn't
contradict standart [2] and fills holes with zeros too.
[1] Makefile:500
496 KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
497 -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
498 -Werror=implicit-function-declaration -Werror=implicit-int \
499 -Wno-format-security \
500 -std=gnu89
[2] From GCC:
https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html
"When a base standard is specified, the compiler accepts all programs
following that standard plus those using GNU extensions that do not
contradict it."
Thanks
>
>
>
Ah, thanks. We've had a bunch of discussions about these leaks but I
wasn't aware of this.
regards,
dan carpenter
On Sun, Aug 02, 2020 at 03:45:40PM -0700, Joe Perches wrote:
> On Sun, 2020-08-02 at 19:28 -0300, Jason Gunthorpe wrote:
> > On Sun, Aug 02, 2020 at 03:23:58PM -0700, Joe Perches wrote:
> > > On Sun, 2020-08-02 at 19:10 -0300, Jason Gunthorpe wrote:
> > > > On Sat, Aug 01, 2020 at 08:38:33AM +0300, Leon Romanovsky wrote:
> > > >
> > > > > I'm using {} instead of {0} because of this GCC bug.
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> > > >
> > > > This is why the {} extension exists..
> > >
> > > There is no guarantee that the gcc struct initialization {}
> > > extension also zeros padding.
> >
> > We just went over this. Yes there is, C11 requires it.
>
> c11 is not c90. The kernel uses c90.
The kernel already relies on a lot of C11/C99 features and
behaviors. For instance Linus just bumped the minimum compiler version
so that C11's _Generic is usable.
Why do you think this particular part of C11 shouldn't be relied on?
Jason
Hello!
Thanks to Jason for getting this conversation back on track.
Yes: in general, {} or a partial initializer /will/ zero padding bits.
However, there is a bug in some versions of GCC where {} will /not/ zero
padding bits; actually, Jason's test program in this mail
https://lore.kernel.org/lkml/[email protected]/
has the right ingredients to trigger the bug, but the GCC
versions used are outside of the bug window. :)
For more details on these cases and more (including said GCC bug), see
my paper at:
https://www.nccgroup.com/us/about-us/newsroom-and-events/blog/2019/october/padding-the-struct-how-a-compiler-optimization-can-disclose-stack-memory/
Hopefully this paper can serve as a helpful reference when these cases
are encountered in the kernel.
Thank you.
Jack Leadford
On 8/3/20 4:06 PM, Jason Gunthorpe wrote:
> On Sun, Aug 02, 2020 at 03:45:40PM -0700, Joe Perches wrote:
>> On Sun, 2020-08-02 at 19:28 -0300, Jason Gunthorpe wrote:
>>> On Sun, Aug 02, 2020 at 03:23:58PM -0700, Joe Perches wrote:
>>>> On Sun, 2020-08-02 at 19:10 -0300, Jason Gunthorpe wrote:
>>>>> On Sat, Aug 01, 2020 at 08:38:33AM +0300, Leon Romanovsky wrote:
>>>>>
>>>>>> I'm using {} instead of {0} because of this GCC bug.
>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
>>>>>
>>>>> This is why the {} extension exists..
>>>>
>>>> There is no guarantee that the gcc struct initialization {}
>>>> extension also zeros padding.
>>>
>>> We just went over this. Yes there is, C11 requires it.
>>
>> c11 is not c90. The kernel uses c90.
>
> The kernel already relies on a lot of C11/C99 features and
> behaviors. For instance Linus just bumped the minimum compiler version
> so that C11's _Generic is usable.
>
> Why do you think this particular part of C11 shouldn't be relied on?
>
> Jason
>
>
On Sat, Aug 08, 2020 at 03:57:33PM -0700, Jack Leadford wrote:
> Hello!
>
> Thanks to Jason for getting this conversation back on track.
>
> Yes: in general, {} or a partial initializer /will/ zero padding bits.
>
> However, there is a bug in some versions of GCC where {} will /not/ zero
> padding bits; actually, Jason's test program in this mail
> https://lore.kernel.org/lkml/[email protected]/
> has the right ingredients to trigger the bug, but the GCC
> versions used are outside of the bug window. :)
>
> For more details on these cases and more (including said GCC bug), see my
> paper at:
>
> https://www.nccgroup.com/us/about-us/newsroom-and-events/blog/2019/october/padding-the-struct-how-a-compiler-optimization-can-disclose-stack-memory/
>
> Hopefully this paper can serve as a helpful reference when these cases are
> encountered in the kernel.
I read the paper and didn't find exact GCC version, only remark that it
was before GCC 7.
So my question, why is this case different from any other GCC bugs?
AFAIK, we don't add kernel code to overcome GCC bugs which exist in
specific versions, which already were fixed.
More on that, this paper talks about specific flow which doesn't exist
in the discussed patch.
Thanks
On Sat, Aug 08, 2020 at 03:57:33PM -0700, Jack Leadford wrote:
> Hello!
>
> Thanks to Jason for getting this conversation back on track.
>
> Yes: in general, {} or a partial initializer /will/ zero padding bits.
>
> However, there is a bug in some versions of GCC where {} will /not/ zero
> padding bits; actually, Jason's test program in this mail
> https://lore.kernel.org/lkml/[email protected]/
> has the right ingredients to trigger the bug, but the GCC
> versions used are outside of the bug window. :)
It seems fine, at least Godbolt doesn't show a bug with that code.
Can you share the test that does fail?
This seems like the sort of security sensitive bug that should be
addressed in gcc, not worked around in the kernel code :\
Jason