Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
@ 2020-07-30 19:20 Peilin Ye
  2020-07-30 19:29 ` santosh.shilimkar
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Peilin Ye @ 2020-07-30 19:20 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: Peilin Ye, David S. Miller, Jakub Kicinski, Dan Carpenter,
	Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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: stable@vger.kernel.org
Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
Fixes: bdbe6fbc6a2f ("RDS: recv.c")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-30 19:20 [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get() Peilin Ye
@ 2020-07-30 19:29 ` santosh.shilimkar
  2020-07-31  4:53 ` Leon Romanovsky
  2020-07-31 23:54 ` David Miller
  2 siblings, 0 replies; 29+ messages in thread
From: santosh.shilimkar @ 2020-07-30 19:29 UTC (permalink / raw)
  To: Peilin Ye
  Cc: David S. Miller, Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	Greg Kroah-Hartman, linux-kernel-mentees, netdev, linux-rdma,
	rds-devel, linux-kernel

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: stable@vger.kernel.org
> Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
> Fixes: bdbe6fbc6a2f ("RDS: recv.c")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> 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 <santosh.shilimkar@oracle.com>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-30 19:20 [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get() Peilin Ye
  2020-07-30 19:29 ` santosh.shilimkar
@ 2020-07-31  4:53 ` Leon Romanovsky
  2020-07-31  5:33   ` Greg Kroah-Hartman
  2020-07-31  9:59   ` Dan Carpenter
  2020-07-31 23:54 ` David Miller
  2 siblings, 2 replies; 29+ messages in thread
From: Leon Romanovsky @ 2020-07-31  4:53 UTC (permalink / raw)
  To: Peilin Ye
  Cc: Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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: stable@vger.kernel.org
> Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
> Fixes: bdbe6fbc6a2f ("RDS: recv.c")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> 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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31  4:53 ` Leon Romanovsky
@ 2020-07-31  5:33   ` Greg Kroah-Hartman
  2020-07-31  5:33     ` Greg Kroah-Hartman
  2020-07-31  9:59   ` Dan Carpenter
  1 sibling, 1 reply; 29+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31  5:33 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Peilin Ye, Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31  5:33   ` Greg Kroah-Hartman
@ 2020-07-31  5:33     ` Greg Kroah-Hartman
       [not found]       ` <CAHp75Vdr2HC_ogNhBCxxGut9=Z6pQMFiA0w-268OQv+5unYOTg@mail.gmail.com>
  2020-07-31 14:04       ` Jason Gunthorpe
  0 siblings, 2 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31  5:33 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Peilin Ye, Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
       [not found]       ` <CAHp75Vdr2HC_ogNhBCxxGut9=Z6pQMFiA0w-268OQv+5unYOTg@mail.gmail.com>
@ 2020-07-31  7:00         ` Leon Romanovsky
  2020-07-31  7:05           ` Andy Shevchenko
  0 siblings, 1 reply; 29+ messages in thread
From: Leon Romanovsky @ 2020-07-31  7:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Greg Kroah-Hartman, Peilin Ye, Santosh Shilimkar,
	David S. Miller, Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

On Fri, Jul 31, 2020 at 09:29:27AM +0300, Andy Shevchenko wrote:
> On Friday, July 31, 2020, Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31  7:00         ` Leon Romanovsky
@ 2020-07-31  7:05           ` Andy Shevchenko
  0 siblings, 0 replies; 29+ messages in thread
From: Andy Shevchenko @ 2020-07-31  7:05 UTC (permalink / raw)
  To: Leon Romanovsky, Sakari Ailus
  Cc: Greg Kroah-Hartman, Peilin Ye, Santosh Shilimkar,
	David S. Miller, Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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 <leon@kernel.org> wrote:
> On Fri, Jul 31, 2020 at 09:29:27AM +0300, Andy Shevchenko wrote:
> > On Friday, July 31, 2020, Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31  4:53 ` Leon Romanovsky
  2020-07-31  5:33   ` Greg Kroah-Hartman
@ 2020-07-31  9:59   ` Dan Carpenter
  2020-07-31 11:14     ` Håkon Bugge
  1 sibling, 1 reply; 29+ messages in thread
From: Dan Carpenter @ 2020-07-31  9:59 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Peilin Ye, Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Arnd Bergmann, Greg Kroah-Hartman, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31  9:59   ` Dan Carpenter
@ 2020-07-31 11:14     ` Håkon Bugge
  2020-07-31 11:59       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 29+ messages in thread
From: Håkon Bugge @ 2020-07-31 11:14 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Leon Romanovsky, Peilin Ye, Santosh Shilimkar, David S. Miller,
	Jakub Kicinski, Arnd Bergmann, Greg Kroah-Hartman,
	linux-kernel-mentees, netdev, OFED mailing list, rds-devel,
	linux-kernel



> On 31 Jul 2020, at 11:59, Dan Carpenter <dan.carpenter@oracle.com> 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
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 11:14     ` Håkon Bugge
@ 2020-07-31 11:59       ` Greg Kroah-Hartman
  2020-07-31 12:03         ` Håkon Bugge
  0 siblings, 1 reply; 29+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31 11:59 UTC (permalink / raw)
  To: Håkon Bugge
  Cc: Dan Carpenter, Leon Romanovsky, Peilin Ye, Santosh Shilimkar,
	David S. Miller, Jakub Kicinski, Arnd Bergmann,
	linux-kernel-mentees, netdev, OFED mailing list, rds-devel,
	linux-kernel

On Fri, Jul 31, 2020 at 01:14:09PM +0200, Håkon Bugge wrote:
> 
> 
> > On 31 Jul 2020, at 11:59, Dan Carpenter <dan.carpenter@oracle.com> 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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 11:59       ` Greg Kroah-Hartman
@ 2020-07-31 12:03         ` Håkon Bugge
  0 siblings, 0 replies; 29+ messages in thread
From: Håkon Bugge @ 2020-07-31 12:03 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Dan Carpenter, Leon Romanovsky, Peilin Ye, Santosh Shilimkar,
	David S. Miller, Jakub Kicinski, Arnd Bergmann,
	linux-kernel-mentees, netdev, OFED mailing list, rds-devel,
	linux-kernel



> On 31 Jul 2020, at 13:59, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> On Fri, Jul 31, 2020 at 01:14:09PM +0200, Håkon Bugge wrote:
>> 
>> 
>>> On 31 Jul 2020, at 11:59, Dan Carpenter <dan.carpenter@oracle.com> 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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31  5:33     ` Greg Kroah-Hartman
       [not found]       ` <CAHp75Vdr2HC_ogNhBCxxGut9=Z6pQMFiA0w-268OQv+5unYOTg@mail.gmail.com>
@ 2020-07-31 14:04       ` Jason Gunthorpe
  2020-07-31 14:21         ` Greg Kroah-Hartman
  1 sibling, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-07-31 14:04 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Leon Romanovsky, Peilin Ye, Santosh Shilimkar, David S. Miller,
	Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 14:04       ` Jason Gunthorpe
@ 2020-07-31 14:21         ` Greg Kroah-Hartman
  2020-07-31 14:36           ` Jason Gunthorpe
  0 siblings, 1 reply; 29+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31 14:21 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Leon Romanovsky, Peilin Ye, Santosh Shilimkar, David S. Miller,
	Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 14:21         ` Greg Kroah-Hartman
@ 2020-07-31 14:36           ` Jason Gunthorpe
  2020-07-31 17:19             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-07-31 14:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Leon Romanovsky, Peilin Ye, Santosh Shilimkar, David S. Miller,
	Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 14:36           ` Jason Gunthorpe
@ 2020-07-31 17:19             ` Greg Kroah-Hartman
  2020-07-31 18:27               ` Jason Gunthorpe
  2020-08-01  5:38               ` Leon Romanovsky
  0 siblings, 2 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-31 17:19 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Leon Romanovsky, Peilin Ye, Santosh Shilimkar, David S. Miller,
	Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 17:19             ` Greg Kroah-Hartman
@ 2020-07-31 18:27               ` Jason Gunthorpe
  2020-08-01  8:00                 ` Dan Carpenter
  2020-08-01  5:38               ` Leon Romanovsky
  1 sibling, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-07-31 18:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Leon Romanovsky, Peilin Ye, Santosh Shilimkar, David S. Miller,
	Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-30 19:20 [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get() Peilin Ye
  2020-07-30 19:29 ` santosh.shilimkar
  2020-07-31  4:53 ` Leon Romanovsky
@ 2020-07-31 23:54 ` David Miller
  2 siblings, 0 replies; 29+ messages in thread
From: David Miller @ 2020-07-31 23:54 UTC (permalink / raw)
  To: yepeilin.cs
  Cc: santosh.shilimkar, kuba, dan.carpenter, arnd, gregkh,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

From: Peilin Ye <yepeilin.cs@gmail.com>
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: stable@vger.kernel.org
> Fixes: f037590fff30 ("rds: fix a leak of kernel memory")
> Fixes: bdbe6fbc6a2f ("RDS: recv.c")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>

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.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 17:19             ` Greg Kroah-Hartman
  2020-07-31 18:27               ` Jason Gunthorpe
@ 2020-08-01  5:38               ` Leon Romanovsky
  2020-08-02 22:10                 ` Jason Gunthorpe
  1 sibling, 1 reply; 29+ messages in thread
From: Leon Romanovsky @ 2020-08-01  5:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jason Gunthorpe, Peilin Ye, Santosh Shilimkar, David S. Miller,
	Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-07-31 18:27               ` Jason Gunthorpe
@ 2020-08-01  8:00                 ` Dan Carpenter
  2020-08-01 14:40                   ` Jason Gunthorpe
  0 siblings, 1 reply; 29+ messages in thread
From: Dan Carpenter @ 2020-08-01  8:00 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Greg Kroah-Hartman, Leon Romanovsky, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Arnd Bergmann, linux-kernel-mentees, netdev, linux-rdma,
	rds-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-01  8:00                 ` Dan Carpenter
@ 2020-08-01 14:40                   ` Jason Gunthorpe
  2020-08-03  9:34                     ` Dan Carpenter
  0 siblings, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-08-01 14:40 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Greg Kroah-Hartman, Leon Romanovsky, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Arnd Bergmann, linux-kernel-mentees, netdev, linux-rdma,
	rds-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-01  5:38               ` Leon Romanovsky
@ 2020-08-02 22:10                 ` Jason Gunthorpe
  2020-08-02 22:23                   ` Joe Perches
  0 siblings, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-08-02 22:10 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Greg Kroah-Hartman, Peilin Ye, Santosh Shilimkar,
	David S. Miller, Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-02 22:10                 ` Jason Gunthorpe
@ 2020-08-02 22:23                   ` Joe Perches
  2020-08-02 22:28                     ` Jason Gunthorpe
  0 siblings, 1 reply; 29+ messages in thread
From: Joe Perches @ 2020-08-02 22:23 UTC (permalink / raw)
  To: Jason Gunthorpe, Leon Romanovsky
  Cc: Greg Kroah-Hartman, Peilin Ye, Santosh Shilimkar,
	David S. Miller, Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
	linux-kernel-mentees, netdev, linux-rdma, rds-devel,
	linux-kernel

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.



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-02 22:23                   ` Joe Perches
@ 2020-08-02 22:28                     ` Jason Gunthorpe
  2020-08-02 22:45                       ` Joe Perches
  0 siblings, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-08-02 22:28 UTC (permalink / raw)
  To: Joe Perches
  Cc: Leon Romanovsky, Greg Kroah-Hartman, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-02 22:28                     ` Jason Gunthorpe
@ 2020-08-02 22:45                       ` Joe Perches
  2020-08-03  4:58                         ` Leon Romanovsky
  2020-08-03 23:06                         ` Jason Gunthorpe
  0 siblings, 2 replies; 29+ messages in thread
From: Joe Perches @ 2020-08-02 22:45 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Leon Romanovsky, Greg Kroah-Hartman, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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.




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-02 22:45                       ` Joe Perches
@ 2020-08-03  4:58                         ` Leon Romanovsky
  2020-08-03 23:06                         ` Jason Gunthorpe
  1 sibling, 0 replies; 29+ messages in thread
From: Leon Romanovsky @ 2020-08-03  4:58 UTC (permalink / raw)
  To: Joe Perches
  Cc: Jason Gunthorpe, Greg Kroah-Hartman, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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

>
>
>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-01 14:40                   ` Jason Gunthorpe
@ 2020-08-03  9:34                     ` Dan Carpenter
  0 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2020-08-03  9:34 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Greg Kroah-Hartman, Leon Romanovsky, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Arnd Bergmann, linux-kernel-mentees, netdev, linux-rdma,
	rds-devel, linux-kernel

Ah, thanks.  We've had a bunch of discussions about these leaks but I
wasn't aware of this.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-02 22:45                       ` Joe Perches
  2020-08-03  4:58                         ` Leon Romanovsky
@ 2020-08-03 23:06                         ` Jason Gunthorpe
  2020-08-08 22:57                           ` Jack Leadford
  1 sibling, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-08-03 23:06 UTC (permalink / raw)
  To: Joe Perches
  Cc: Leon Romanovsky, Greg Kroah-Hartman, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-03 23:06                         ` Jason Gunthorpe
@ 2020-08-08 22:57                           ` Jack Leadford
  2020-08-09  7:04                             ` Leon Romanovsky
  0 siblings, 1 reply; 29+ messages in thread
From: Jack Leadford @ 2020-08-08 22:57 UTC (permalink / raw)
  To: Jason Gunthorpe, Joe Perches
  Cc: Leon Romanovsky, Greg Kroah-Hartman, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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/20200731143604.GF24045@ziepe.ca/
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
> 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
  2020-08-08 22:57                           ` Jack Leadford
@ 2020-08-09  7:04                             ` Leon Romanovsky
  0 siblings, 0 replies; 29+ messages in thread
From: Leon Romanovsky @ 2020-08-09  7:04 UTC (permalink / raw)
  To: Jack Leadford
  Cc: Jason Gunthorpe, Joe Perches, Greg Kroah-Hartman, Peilin Ye,
	Santosh Shilimkar, David S. Miller, Jakub Kicinski,
	Dan Carpenter, Arnd Bergmann, linux-kernel-mentees, netdev,
	linux-rdma, rds-devel, linux-kernel

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/20200731143604.GF24045@ziepe.ca/
> 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

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30 19:20 [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get() Peilin Ye
2020-07-30 19:29 ` santosh.shilimkar
2020-07-31  4:53 ` Leon Romanovsky
2020-07-31  5:33   ` Greg Kroah-Hartman
2020-07-31  5:33     ` Greg Kroah-Hartman
     [not found]       ` <CAHp75Vdr2HC_ogNhBCxxGut9=Z6pQMFiA0w-268OQv+5unYOTg@mail.gmail.com>
2020-07-31  7:00         ` Leon Romanovsky
2020-07-31  7:05           ` Andy Shevchenko
2020-07-31 14:04       ` Jason Gunthorpe
2020-07-31 14:21         ` Greg Kroah-Hartman
2020-07-31 14:36           ` Jason Gunthorpe
2020-07-31 17:19             ` Greg Kroah-Hartman
2020-07-31 18:27               ` Jason Gunthorpe
2020-08-01  8:00                 ` Dan Carpenter
2020-08-01 14:40                   ` Jason Gunthorpe
2020-08-03  9:34                     ` Dan Carpenter
2020-08-01  5:38               ` Leon Romanovsky
2020-08-02 22:10                 ` Jason Gunthorpe
2020-08-02 22:23                   ` Joe Perches
2020-08-02 22:28                     ` Jason Gunthorpe
2020-08-02 22:45                       ` Joe Perches
2020-08-03  4:58                         ` Leon Romanovsky
2020-08-03 23:06                         ` Jason Gunthorpe
2020-08-08 22:57                           ` Jack Leadford
2020-08-09  7:04                             ` Leon Romanovsky
2020-07-31  9:59   ` Dan Carpenter
2020-07-31 11:14     ` Håkon Bugge
2020-07-31 11:59       ` Greg Kroah-Hartman
2020-07-31 12:03         ` Håkon Bugge
2020-07-31 23:54 ` David Miller

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git