linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fs: fix possible Spectre V1 indexing in __close_fd()
@ 2018-09-24 18:15 Greg Hackmann
  2018-09-24 18:39 ` Greg KH
  0 siblings, 1 reply; 4+ messages in thread
From: Greg Hackmann @ 2018-09-24 18:15 UTC (permalink / raw)
  To: Alexander Viro
  Cc: Omer Tripp, linux-fsdevel, linux-kernel, Greg Hackmann, stable

__close_fd() is reachable via the close() syscall with a
userspace-controlled fd.  Ensure that it can't be used to speculatively
access past the end of current->fdt.

Reported-by: Omer Tripp <trippo@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Greg Hackmann <ghackmann@google.com>
---
 fs/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/file.c b/fs/file.c
index 7ffd6e9d103d..a80cf82be96b 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -18,6 +18,7 @@
 #include <linux/bitops.h>
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
+#include <linux/nospec.h>
 
 unsigned int sysctl_nr_open __read_mostly = 1024*1024;
 unsigned int sysctl_nr_open_min = BITS_PER_LONG;
@@ -626,6 +627,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
 	fdt = files_fdtable(files);
 	if (fd >= fdt->max_fds)
 		goto out_unlock;
+	fd = array_index_nospec(fd, fdt->max_fds);
 	file = fdt->fd[fd];
 	if (!file)
 		goto out_unlock;
-- 
2.19.0.397.gdd90340f6a-goog

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

* Re: [PATCH] fs: fix possible Spectre V1 indexing in __close_fd()
  2018-09-24 18:15 [PATCH] fs: fix possible Spectre V1 indexing in __close_fd() Greg Hackmann
@ 2018-09-24 18:39 ` Greg KH
  2018-10-15 13:37   ` Greg KH
  0 siblings, 1 reply; 4+ messages in thread
From: Greg KH @ 2018-09-24 18:39 UTC (permalink / raw)
  To: Greg Hackmann
  Cc: Alexander Viro, Omer Tripp, linux-fsdevel, linux-kernel,
	Greg Hackmann, stable

On Mon, Sep 24, 2018 at 11:15:00AM -0700, Greg Hackmann wrote:
> __close_fd() is reachable via the close() syscall with a
> userspace-controlled fd.  Ensure that it can't be used to speculatively
> access past the end of current->fdt.
> 
> Reported-by: Omer Tripp <trippo@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Greg Hackmann <ghackmann@google.com>
> ---
>  fs/file.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 7ffd6e9d103d..a80cf82be96b 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -18,6 +18,7 @@
>  #include <linux/bitops.h>
>  #include <linux/spinlock.h>
>  #include <linux/rcupdate.h>
> +#include <linux/nospec.h>
>  
>  unsigned int sysctl_nr_open __read_mostly = 1024*1024;
>  unsigned int sysctl_nr_open_min = BITS_PER_LONG;
> @@ -626,6 +627,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
>  	fdt = files_fdtable(files);
>  	if (fd >= fdt->max_fds)
>  		goto out_unlock;
> +	fd = array_index_nospec(fd, fdt->max_fds);
>  	file = fdt->fd[fd];

Don't you need 2 "halfs" of a gadget in order to make it work?  This is
one half, where is the second half?

Or am I reading this code wrong here somehow?

We don't want to play whack-a-mole with only 1/2 spectre gadgets,
otherwise the 700+ patches that Red Hat added to their kernel would have
been merged already.

Which reminds me, did the Red Hat tooling catch this one as well?  If
not, someone need to go fix it :)

thanks,

greg k-h

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

* Re: [PATCH] fs: fix possible Spectre V1 indexing in __close_fd()
  2018-09-24 18:39 ` Greg KH
@ 2018-10-15 13:37   ` Greg KH
       [not found]     ` <CALTzUognuG-N4M5w9xugxF7KZjUxF_9O32fKoUt2v7st70ppmw@mail.gmail.com>
  0 siblings, 1 reply; 4+ messages in thread
From: Greg KH @ 2018-10-15 13:37 UTC (permalink / raw)
  To: Greg Hackmann
  Cc: Alexander Viro, Omer Tripp, linux-fsdevel, linux-kernel,
	Greg Hackmann, stable

On Mon, Sep 24, 2018 at 08:39:11PM +0200, Greg KH wrote:
> On Mon, Sep 24, 2018 at 11:15:00AM -0700, Greg Hackmann wrote:
> > __close_fd() is reachable via the close() syscall with a
> > userspace-controlled fd.  Ensure that it can't be used to speculatively
> > access past the end of current->fdt.
> > 
> > Reported-by: Omer Tripp <trippo@google.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Greg Hackmann <ghackmann@google.com>
> > ---
> >  fs/file.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index 7ffd6e9d103d..a80cf82be96b 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/bitops.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/rcupdate.h>
> > +#include <linux/nospec.h>
> >  
> >  unsigned int sysctl_nr_open __read_mostly = 1024*1024;
> >  unsigned int sysctl_nr_open_min = BITS_PER_LONG;
> > @@ -626,6 +627,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
> >  	fdt = files_fdtable(files);
> >  	if (fd >= fdt->max_fds)
> >  		goto out_unlock;
> > +	fd = array_index_nospec(fd, fdt->max_fds);
> >  	file = fdt->fd[fd];
> 
> Don't you need 2 "halfs" of a gadget in order to make it work?  This is
> one half, where is the second half?
> 
> Or am I reading this code wrong here somehow?
> 
> We don't want to play whack-a-mole with only 1/2 spectre gadgets,
> otherwise the 700+ patches that Red Hat added to their kernel would have
> been merged already.
> 
> Which reminds me, did the Red Hat tooling catch this one as well?  If
> not, someone need to go fix it :)

Ping?

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

* Re: [PATCH] fs: fix possible Spectre V1 indexing in __close_fd()
       [not found]     ` <CALTzUognuG-N4M5w9xugxF7KZjUxF_9O32fKoUt2v7st70ppmw@mail.gmail.com>
@ 2018-12-19  7:11       ` Greg KH
  0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2018-12-19  7:11 UTC (permalink / raw)
  To: Omer Tripp
  Cc: ghackmann, viro, linux-fsdevel, linux-kernel, Greg Hackmann, stable

On Mon, Oct 15, 2018 at 06:54:31AM -0700, Omer Tripp wrote:
> Hi Greg and all,
> 
> Here is my analysis of the complete gadget, and looking forward to your
> corrections/feedback if there are any inaccuracies:
> 
> 
>    1.
> 
>    __close_fd() is reachable via the close() syscall with a user-controlled
>    fd.
>    2.
> 
>    If said bounds check is mispredicted, then a user-controlled address
>    fdt->fd[fd] is obtained then dereferenced, and the value of a
>    user-controlled address is loaded into the local variable file.
>    3.
> 
>    file is then passed as an argument to filp_close, where the cache
> lines secret
>    + offsetof(f_op) and secret + offsetof(f_mode) are hot and vulnerable to
>    a timing channel attack.
> 
> 
> The mitigation proposed by Greg Hackmann blocks this gadget.

What ever happened to this patch?  Did it get reposted?  If not, can
someone please do so with this text in the changelog?

thanks,

greg k-h

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

end of thread, other threads:[~2018-12-19  7:11 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-24 18:15 [PATCH] fs: fix possible Spectre V1 indexing in __close_fd() Greg Hackmann
2018-09-24 18:39 ` Greg KH
2018-10-15 13:37   ` Greg KH
     [not found]     ` <CALTzUognuG-N4M5w9xugxF7KZjUxF_9O32fKoUt2v7st70ppmw@mail.gmail.com>
2018-12-19  7:11       ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).