Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Alexey Dobriyan]

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

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Matthew Wilcox]

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?

[PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[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

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Michal Hocko]

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

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Dianzhang Chen]

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

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Michal Hocko]

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

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Dianzhang Chen]

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

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Michal Hocko]

[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

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Dianzhang Chen]

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.

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Matthew Wilcox]

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)

Re: [PATCH] mm/slab_common.c: fix possible spectre-v1 in kmalloc_slab()

[Dianzhang Chen]

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)