LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
@ 2019-05-29 12:37 Dianzhang Chen
  2019-05-29 16:25 ` Michal Hocko
  2019-05-29 19:48 ` Matthew Wilcox
  0 siblings, 2 replies; 11+ messages in thread
From: Dianzhang Chen @ 2019-05-29 12:37 UTC (permalink / raw)
  To: cl
  Cc: penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, linux-kernel,
	Dianzhang Chen

The `size` in kmalloc_slab() is indirectly controlled by userspace via syscall: poll(defined in fs/select.c), hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
The `size` can be controlled from: poll -> do_sys_poll -> kmalloc -> __kmalloc -> kmalloc_slab.

Fix this by sanitizing `size` before using it to index size_index.

Signed-off-by: Dianzhang Chen <dianzhangchen0@gmail.com>
---
 mm/slab_common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 58251ba..41c7e34 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -21,6 +21,7 @@
 #include <asm/tlbflush.h>
 #include <asm/page.h>
 #include <linux/memcontrol.h>
+#include <linux/nospec.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/kmem.h>
@@ -1056,6 +1057,7 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 		if (!size)
 			return ZERO_SIZE_PTR;
 
+		size = array_index_nospec(size, 193);
 		index = size_index[size_index_elem(size)];
 	} else {
 		if (WARN_ON_ONCE(size > KMALLOC_MAX_CACHE_SIZE))
-- 
2.7.4


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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-29 12:37 [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab() Dianzhang Chen
@ 2019-05-29 16:25 ` Michal Hocko
  2019-05-29 16:39   ` Dianzhang Chen
  2019-05-29 19:48 ` Matthew Wilcox
  1 sibling, 1 reply; 11+ messages in thread
From: Michal Hocko @ 2019-05-29 16:25 UTC (permalink / raw)
  To: Dianzhang Chen
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, linux-kernel

On Wed 29-05-19 20:37:28, Dianzhang Chen wrote:
[...]
> @@ -1056,6 +1057,7 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
>  		if (!size)
>  			return ZERO_SIZE_PTR;
>  
> +		size = array_index_nospec(size, 193);
>  		index = size_index[size_index_elem(size)];

What is this 193 magic number?
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-29 16:25 ` Michal Hocko
@ 2019-05-29 16:39   ` Dianzhang Chen
  2019-05-29 17:49     ` Michal Hocko
  0 siblings, 1 reply; 11+ messages in thread
From: Dianzhang Chen @ 2019-05-29 16:39 UTC (permalink / raw)
  To: Michal Hocko; +Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, LKML

It's come from `192+1`.


The more code fragment is:


if (size <= 192) {

    if (!size)

        return ZERO_SIZE_PTR;

    size = array_index_nospec(size, 193);

    index = size_index[size_index_elem(size)];

}


Sine array_index_nospec(index, size) can clamp the index within the
range of [0, size), so in order to make the `size<=192`, need to clamp
the index in the range of [0, 192+1) .

On Thu, May 30, 2019 at 12:25 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Wed 29-05-19 20:37:28, Dianzhang Chen wrote:
> [...]
> > @@ -1056,6 +1057,7 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
> >               if (!size)
> >                       return ZERO_SIZE_PTR;
> >
> > +             size = array_index_nospec(size, 193);
> >               index = size_index[size_index_elem(size)];
>
> What is this 193 magic number?
> --
> Michal Hocko
> SUSE Labs

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-29 16:39   ` Dianzhang Chen
@ 2019-05-29 17:49     ` Michal Hocko
  2019-05-30  5:20       ` Dianzhang Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Michal Hocko @ 2019-05-29 17:49 UTC (permalink / raw)
  To: Dianzhang Chen
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, LKML

On Thu 30-05-19 00:39:53, Dianzhang Chen wrote:
> It's come from `192+1`.
> 
> 
> The more code fragment is:
> 
> 
> if (size <= 192) {
> 
>     if (!size)
> 
>         return ZERO_SIZE_PTR;
> 
>     size = array_index_nospec(size, 193);
> 
>     index = size_index[size_index_elem(size)];
> 
> }

OK I see, I could have looked into the code, my bad. But I am still not
sure what is the potential exploit scenario and why this particular path
a needs special treatment while other size branches are ok. Could you be
more specific please?
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-29 12:37 [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab() Dianzhang Chen
  2019-05-29 16:25 ` Michal Hocko
@ 2019-05-29 19:48 ` Matthew Wilcox
  2019-05-30  5:21   ` Dianzhang Chen
  1 sibling, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2019-05-29 19:48 UTC (permalink / raw)
  To: Dianzhang Chen
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, linux-kernel

On Wed, May 29, 2019 at 08:37:28PM +0800, Dianzhang Chen wrote:
> The `size` in kmalloc_slab() is indirectly controlled by userspace via syscall: poll(defined in fs/select.c), hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
> The `size` can be controlled from: poll -> do_sys_poll -> kmalloc -> __kmalloc -> kmalloc_slab.
> 
> Fix this by sanitizing `size` before using it to index size_index.

I think it makes more sense to sanitize size in size_index_elem(),
don't you?

 static inline unsigned int size_index_elem(unsigned int bytes)
 {
-	return (bytes - 1) / 8;
+	return array_index_nospec((bytes - 1) / 8, ARRAY_SIZE(size_index));
 }

(untested)

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-29 17:49     ` Michal Hocko
@ 2019-05-30  5:20       ` Dianzhang Chen
  2019-05-30  6:24         ` Michal Hocko
  0 siblings, 1 reply; 11+ messages in thread
From: Dianzhang Chen @ 2019-05-30  5:20 UTC (permalink / raw)
  To: Michal Hocko; +Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, LKML

It is possible that a CPU mis-predicts the conditional branch, and
speculatively loads size_index[size_index_elem(size)], even if size >192.
Although this value will subsequently be discarded,
but it can not drop all the effects of speculative execution,
such as the presence or absence of data in caches. Such effects may
form side-channels which can be
observed to extract secret information.


As for "why this particular path a needs special treatment while other
size branches are ok",
i think the other size branches need to treatment as well at first place,
but in code `index = fls(size - 1)` the function `fls` will make the
index at specific range,
so it can not use `kmalloc_caches[kmalloc_type(flags)][index]` to load
arbitury data.
But, still it may load some date that it shouldn't, if necessary, i
think can add array_index_nospec as well.



On Thu, May 30, 2019 at 1:49 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Thu 30-05-19 00:39:53, Dianzhang Chen wrote:
> > It's come from `192+1`.
> >
> >
> > The more code fragment is:
> >
> >
> > if (size <= 192) {
> >
> >     if (!size)
> >
> >         return ZERO_SIZE_PTR;
> >
> >     size = array_index_nospec(size, 193);
> >
> >     index = size_index[size_index_elem(size)];
> >
> > }
>
> OK I see, I could have looked into the code, my bad. But I am still not
> sure what is the potential exploit scenario and why this particular path
> a needs special treatment while other size branches are ok. Could you be
> more specific please?
> --
> Michal Hocko
> SUSE Labs

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-29 19:48 ` Matthew Wilcox
@ 2019-05-30  5:21   ` Dianzhang Chen
  0 siblings, 0 replies; 11+ messages in thread
From: Dianzhang Chen @ 2019-05-30  5:21 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, LKML

thanks, i think your suggestion is ok.
in my previous method is easy to understand for spectre  logic,
but your suggestion is more sense to use of array_index_nospec.



On Thu, May 30, 2019 at 3:48 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, May 29, 2019 at 08:37:28PM +0800, Dianzhang Chen wrote:
> > The `size` in kmalloc_slab() is indirectly controlled by userspace via syscall: poll(defined in fs/select.c), hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
> > The `size` can be controlled from: poll -> do_sys_poll -> kmalloc -> __kmalloc -> kmalloc_slab.
> >
> > Fix this by sanitizing `size` before using it to index size_index.
>
> I think it makes more sense to sanitize size in size_index_elem(),
> don't you?
>
>  static inline unsigned int size_index_elem(unsigned int bytes)
>  {
> -       return (bytes - 1) / 8;
> +       return array_index_nospec((bytes - 1) / 8, ARRAY_SIZE(size_index));
>  }
>
> (untested)

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-30  5:20       ` Dianzhang Chen
@ 2019-05-30  6:24         ` Michal Hocko
  2019-05-30  7:01           ` Dianzhang Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Michal Hocko @ 2019-05-30  6:24 UTC (permalink / raw)
  To: Dianzhang Chen
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, LKML

[Please do not top-post]

On Thu 30-05-19 13:20:01, Dianzhang Chen wrote:
> It is possible that a CPU mis-predicts the conditional branch, and
> speculatively loads size_index[size_index_elem(size)], even if size >192.
> Although this value will subsequently be discarded,
> but it can not drop all the effects of speculative execution,
> such as the presence or absence of data in caches. Such effects may
> form side-channels which can be
> observed to extract secret information.

I understand the general mechanism of spectre v1. What I was asking for
is an example of where userspace directly controls the allocation size
as this is usually bounded to an in kernel object size. I can see how
and N * sizeof(object) where N is controlled by the userspace could be
the target. But calling that out explicitly would be appreciated.
 
> As for "why this particular path a needs special treatment while other
> size branches are ok",
> i think the other size branches need to treatment as well at first place,
> but in code `index = fls(size - 1)` the function `fls` will make the
> index at specific range,
> so it can not use `kmalloc_caches[kmalloc_type(flags)][index]` to load
> arbitury data.
> But, still it may load some date that it shouldn't, if necessary, i
> think can add array_index_nospec as well.

Please mention that in the changelog as well.
 
> On Thu, May 30, 2019 at 1:49 AM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Thu 30-05-19 00:39:53, Dianzhang Chen wrote:
> > > It's come from `192+1`.
> > >
> > >
> > > The more code fragment is:
> > >
> > >
> > > if (size <= 192) {
> > >
> > >     if (!size)
> > >
> > >         return ZERO_SIZE_PTR;
> > >
> > >     size = array_index_nospec(size, 193);
> > >
> > >     index = size_index[size_index_elem(size)];
> > >
> > > }
> >
> > OK I see, I could have looked into the code, my bad. But I am still not
> > sure what is the potential exploit scenario and why this particular path
> > a needs special treatment while other size branches are ok. Could you be
> > more specific please?
> > --
> > Michal Hocko
> > SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-30  6:24         ` Michal Hocko
@ 2019-05-30  7:01           ` Dianzhang Chen
  0 siblings, 0 replies; 11+ messages in thread
From: Dianzhang Chen @ 2019-05-30  7:01 UTC (permalink / raw)
  To: Michal Hocko; +Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, LKML

On Thu, May 30, 2019 at 2:24 PM Michal Hocko <mhocko@kernel.org> wrote:
> I understand the general mechanism of spectre v1. What I was asking for
> is an example of where userspace directly controls the allocation size
> as this is usually bounded to an in kernel object size. I can see how
> and N * sizeof(object) where N is controlled by the userspace could be
> the target. But calling that out explicitly would be appreciated.

In the syscall call poll, the user can control the `nfds`,
when call the function `do_sys_poll` it can pass the nfds to function
`do_sys_poll`, and pass to variable `len`,
although there exit compare instruction llike `len = min_t(unsigned
int, nfds, N_STACK_PPS)`, `len = min(todo, POLLFD_PER_PAGE);`,
but it can also bypass by speculation, as the speculation windows are large,
and in the next `size = sizeof(struct poll_list) + sizeof(struct pollfd) * len`,
which can indirect control the size.


> Please mention that in the changelog as well.
ok, thanks for suggestion.

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
  2019-05-29 20:31 Alexey Dobriyan
@ 2019-05-30  0:04 ` Matthew Wilcox
  0 siblings, 0 replies; 11+ messages in thread
From: Matthew Wilcox @ 2019-05-30  0:04 UTC (permalink / raw)
  To: Alexey Dobriyan; +Cc: dianzhangchen0, linux-kernel, mhocko

On Wed, May 29, 2019 at 11:31:06PM +0300, Alexey Dobriyan wrote:
> > I think it makes more sense to sanitize size in size_index_elem(),
> > don't you?
> 
> > -	return (bytes - 1) / 8;
> > +	return array_index_nospec((bytes - 1) / 8, ARRAY_SIZE(size_index));
> 
> I think it should be fixed in poll.
> Literally every small variable kmalloc call is going through this function.

We could do that too, but don't we then have to audit every ioctl and
similar to see if there's a k(v)malloc based on a size passed from
userspace?

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

* Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()
@ 2019-05-29 20:31 Alexey Dobriyan
  2019-05-30  0:04 ` Matthew Wilcox
  0 siblings, 1 reply; 11+ messages in thread
From: Alexey Dobriyan @ 2019-05-29 20:31 UTC (permalink / raw)
  To: dianzhangchen0; +Cc: linux-kernel, mhocko, willy

> I think it makes more sense to sanitize size in size_index_elem(),
> don't you?

> -	return (bytes - 1) / 8;
> +	return array_index_nospec((bytes - 1) / 8, ARRAY_SIZE(size_index));

I think it should be fixed in poll.
Literally every small variable kmalloc call is going through this function.

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

end of thread, other threads:[~2019-05-30  7:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-29 12:37 [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab() Dianzhang Chen
2019-05-29 16:25 ` Michal Hocko
2019-05-29 16:39   ` Dianzhang Chen
2019-05-29 17:49     ` Michal Hocko
2019-05-30  5:20       ` Dianzhang Chen
2019-05-30  6:24         ` Michal Hocko
2019-05-30  7:01           ` Dianzhang Chen
2019-05-29 19:48 ` Matthew Wilcox
2019-05-30  5:21   ` Dianzhang Chen
2019-05-29 20:31 Alexey Dobriyan
2019-05-30  0:04 ` Matthew Wilcox

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).