From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: rds-devel@oss.oracle.com, Arnd Bergmann <arnd@arndb.de>,
Leon Romanovsky <leon@kernel.org>,
linux-rdma@vger.kernel.org,
Santosh Shilimkar <santosh.shilimkar@oracle.com>,
linux-kernel@vger.kernel.org,
"David S. Miller" <davem@davemloft.net>,
netdev@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
linux-kernel-mentees@lists.linuxfoundation.org,
Peilin Ye <yepeilin.cs@gmail.com>
Subject: Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
Date: Sat, 1 Aug 2020 11:00:26 +0300 [thread overview]
Message-ID: <20200801080026.GJ5493@kadam> (raw)
In-Reply-To: <20200731182712.GI24045@ziepe.ca>
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Leon Romanovsky <leon@kernel.org>,
Peilin Ye <yepeilin.cs@gmail.com>,
Santosh Shilimkar <santosh.shilimkar@oracle.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
linux-kernel-mentees@lists.linuxfoundation.org,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
rds-devel@oss.oracle.com, linux-kernel@vger.kernel.org
Subject: Re: [Linux-kernel-mentees] [PATCH net] rds: Prevent kernel-infoleak in rds_notify_queue_get()
Date: Sat, 1 Aug 2020 11:00:26 +0300 [thread overview]
Message-ID: <20200801080026.GJ5493@kadam> (raw)
In-Reply-To: <20200731182712.GI24045@ziepe.ca>
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
next prev parent reply other threads:[~2020-08-01 8:01 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
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:20 ` Peilin Ye
2020-07-30 19:29 ` santosh.shilimkar
2020-07-30 19:29 ` santosh.shilimkar
2020-07-31 4:53 ` Leon Romanovsky
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 5:33 ` Greg Kroah-Hartman
2020-07-31 5:33 ` Greg Kroah-Hartman
2020-07-31 6:29 ` Andy Shevchenko
2020-07-31 7:00 ` Leon Romanovsky
2020-07-31 7:00 ` Leon Romanovsky
2020-07-31 7:05 ` Andy Shevchenko
2020-07-31 7:05 ` Andy Shevchenko
2020-07-31 14:04 ` Jason Gunthorpe
2020-07-31 14:04 ` Jason Gunthorpe
2020-07-31 14:21 ` Greg Kroah-Hartman
2020-07-31 14:21 ` Greg Kroah-Hartman
2020-07-31 14:36 ` Jason Gunthorpe
2020-07-31 14:36 ` Jason Gunthorpe
2020-07-31 17:19 ` Greg Kroah-Hartman
2020-07-31 17:19 ` Greg Kroah-Hartman
2020-07-31 18:27 ` Jason Gunthorpe
2020-07-31 18:27 ` Jason Gunthorpe
2020-08-01 8:00 ` Dan Carpenter [this message]
2020-08-01 8:00 ` Dan Carpenter
2020-08-01 14:40 ` Jason Gunthorpe
2020-08-01 14:40 ` Jason Gunthorpe
2020-08-03 9:34 ` Dan Carpenter
2020-08-03 9:34 ` Dan Carpenter
2020-08-01 5:38 ` Leon Romanovsky
2020-08-01 5:38 ` Leon Romanovsky
2020-08-02 22:10 ` Jason Gunthorpe
2020-08-02 22:10 ` Jason Gunthorpe
2020-08-02 22:23 ` Joe Perches
2020-08-02 22:23 ` Joe Perches
2020-08-02 22:28 ` Jason Gunthorpe
2020-08-02 22:28 ` Jason Gunthorpe
2020-08-02 22:45 ` Joe Perches
2020-08-02 22:45 ` Joe Perches
2020-08-03 4:58 ` Leon Romanovsky
2020-08-03 4:58 ` Leon Romanovsky
2020-08-03 23:06 ` Jason Gunthorpe
2020-08-03 23:06 ` Jason Gunthorpe
2020-08-08 22:57 ` Jack Leadford
2020-08-08 22:57 ` Jack Leadford
2020-08-09 7:04 ` Leon Romanovsky
2020-08-09 7:04 ` Leon Romanovsky
2020-08-14 17:07 ` Jason Gunthorpe
2020-08-14 17:07 ` Jason Gunthorpe
2020-07-31 6:31 ` Andy Shevchenko
2020-07-31 9:59 ` Dan Carpenter
2020-07-31 9:59 ` Dan Carpenter
2020-07-31 11:14 ` Håkon Bugge
2020-07-31 11:14 ` Håkon Bugge
2020-07-31 11:59 ` Greg Kroah-Hartman
2020-07-31 11:59 ` Greg Kroah-Hartman
2020-07-31 12:03 ` Håkon Bugge
2020-07-31 12:03 ` Håkon Bugge
2020-07-31 23:54 ` David Miller
2020-07-31 23:54 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200801080026.GJ5493@kadam \
--to=dan.carpenter@oracle.com \
--cc=arnd@arndb.de \
--cc=davem@davemloft.net \
--cc=jgg@ziepe.ca \
--cc=kuba@kernel.org \
--cc=leon@kernel.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=rds-devel@oss.oracle.com \
--cc=santosh.shilimkar@oracle.com \
--cc=yepeilin.cs@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.