Re: [PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

Re: [PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

[Cyrill Gorcunov]

On Tue, May 28, 2019 at 10:37:10AM +0800, Dianzhang Chen wrote:
> Hi,
> Because when i reply your email，i always get 'Message rejected' from
> gmail(get this rejection from all the recipients). I still don't know
> how to deal with it, so i reply your email here:

Hi! This is weird. Next time simply reply to LKML (I CC'ed it back).

> Because of speculative execution, the attacker can bypass the bound
> check `if (resource >= RLIM_NLIMITS)`.

And then misprediction get detected and execution is dropped. So I
still don't see a problem here, since we don't leak info even in
such case.

That said I don't mind for this patch but rather in a sake of
code clarity, not because of spectre issue since it has
nothing to do here.

> as for array_index_nospec(index, size), it will clamp the index within
> the range of [0, size), and attacker can't exploit speculative
> execution to make the index out of range [0, size).
> 
> 
> For more detail, please check the link below:
> 
> https://github.com/torvalds/linux/commit/f3804203306e098dae9ca51540fcd5eb700d7f40

Re: [PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

[Dianzhang Chen]

Hi,

Although when detect it is misprediction and drop the execution, but
it can not drop all the effects of speculative execution, like the
cache state. During the speculative execution, the:


rlim = tsk->signal->rlim + resource;    // use resource as index

...

            *old_rlim = *rlim;


may read some secret data into cache.

and then the attacker can use side-channel attack to find out what the
secret data is.


Virtually any observable effect of speculatively executed code can be
leveraged to create the covert channel that leaks sensitive
information[1].


A general form of spectre v1 would be[1]:

if (x < array1_size) {

    y = array1[x];

    // do something using y that is

    // observable when speculatively

    // executed

}


[1] https://spectreattack.com/spectre.pdf

Cyrill Gorcunov <gorcunov@gmail.com> 于2019年5月28日周二 下午3:10写道：
>
> On Tue, May 28, 2019 at 10:37:10AM +0800, Dianzhang Chen wrote:
> > Hi,
> > Because when i reply your email，i always get 'Message rejected' from
> > gmail(get this rejection from all the recipients). I still don't know
> > how to deal with it, so i reply your email here:
>
> Hi! This is weird. Next time simply reply to LKML (I CC'ed it back).
>
> > Because of speculative execution, the attacker can bypass the bound
> > check `if (resource >= RLIM_NLIMITS)`.
>
> And then misprediction get detected and execution is dropped. So I
> still don't see a problem here, since we don't leak info even in
> such case.
>
> That said I don't mind for this patch but rather in a sake of
> code clarity, not because of spectre issue since it has
> nothing to do here.
>
> > as for array_index_nospec(index, size), it will clamp the index within
> > the range of [0, size), and attacker can't exploit speculative
> > execution to make the index out of range [0, size).
> >
> >
> > For more detail, please check the link below:
> >
> > https://github.com/torvalds/linux/commit/f3804203306e098dae9ca51540fcd5eb700d7f40

Re: [PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

[Cyrill Gorcunov]

On Wed, May 29, 2019 at 10:39:52AM +0800, Dianzhang Chen wrote:
> Hi,
> 
> Although when detect it is misprediction and drop the execution, but
> it can not drop all the effects of speculative execution, like the
> cache state. During the speculative execution, the:
> 
> 
> rlim = tsk->signal->rlim + resource;    // use resource as index
> 
> ...
> 
>             *old_rlim = *rlim;
> 
> 
> may read some secret data into cache.
> 
> and then the attacker can use side-channel attack to find out what the
> secret data is.

This code works after check_prlimit_permission call, which means you already
should have a permission granted. And you implies that misprediction gonna
be that deep which involves a number of calls/read/writes/jumps/locks-rb-wb-flushes
and a bunch or other instructions, moreover all conditions are "mispredicted".
This is very bold and actually unproved claim!

Note that I pointed the patch is fine in cleanup context but seriously I
don't see how this all can be exploitable in sense of spectre.

Re: [PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

[Dianzhang Chen]

Though syscall `getrlimit` , it seems not works after check_prlimit_permission.

And the speculation windows are large, as said[1]:
>> Can the speculation proceed past the task_lock()?  Or is the policy to
>> ignore such happy happenstances even if they are available?
>
> Locks are not in the way of speculation. Speculation has almost no limits
> except serializing instructions. At least they respect the magic AND
> limitation in array_index_nospec().

[1] https://do-db2.lkml.org/lkml/2018/5/15/1056

On Wed, May 29, 2019 at 8:18 PM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Wed, May 29, 2019 at 10:39:52AM +0800, Dianzhang Chen wrote:
> > Hi,
> >
> > Although when detect it is misprediction and drop the execution, but
> > it can not drop all the effects of speculative execution, like the
> > cache state. During the speculative execution, the:
> >
> >
> > rlim = tsk->signal->rlim + resource;    // use resource as index
> >
> > ...
> >
> >             *old_rlim = *rlim;
> >
> >
> > may read some secret data into cache.
> >
> > and then the attacker can use side-channel attack to find out what the
> > secret data is.
>
> This code works after check_prlimit_permission call, which means you already
> should have a permission granted. And you implies that misprediction gonna
> be that deep which involves a number of calls/read/writes/jumps/locks-rb-wb-flushes
> and a bunch or other instructions, moreover all conditions are "mispredicted".
> This is very bold and actually unproved claim!
>
> Note that I pointed the patch is fine in cleanup context but seriously I
> don't see how this all can be exploitable in sense of spectre.

Re: [PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

[Cyrill Gorcunov]

On Thu, May 30, 2019 at 01:45:16PM +0800, Dianzhang Chen wrote:
>
> Though syscall `getrlimit` , it seems not works after check_prlimit_permission.
> 
> And the speculation windows are large, as said[1]:
> >> Can the speculation proceed past the task_lock()?  Or is the policy to
> >> ignore such happy happenstances even if they are available?
> >
> > Locks are not in the way of speculation. Speculation has almost no limits
> > except serializing instructions. At least they respect the magic AND
> > limitation in array_index_nospec().
> 
> [1] https://do-db2.lkml.org/lkml/2018/5/15/1056

Please, stop top-posting, it trashes conversation context. You miss the point:
before bpu hits misprediction we've a number of branches, second in case of
misprediction the kernel's stack value is cached, so I'm not convinced at all
that teaching bpu and read the cache is easy here (or possible at all). Thus,
the final solution is up to maintainers. Another reason why I complaining about
the patch -- it is not the patch body, as I said I'm fine with it, but the patch
title: it implies the fix should go in stable kernels, that's what I dont agree
with. But again, I'm not a maintainer and might be wrong.

> 
> On Wed, May 29, 2019 at 8:18 PM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> >
> > On Wed, May 29, 2019 at 10:39:52AM +0800, Dianzhang Chen wrote:
> > > Hi,
> > >
> > > Although when detect it is misprediction and drop the execution, but
> > > it can not drop all the effects of speculative execution, like the
> > > cache state. During the speculative execution, the:
> > >
> > >
> > > rlim = tsk->signal->rlim + resource;    // use resource as index
> > >
> > > ...
> > >
> > >             *old_rlim = *rlim;
> > >
> > >
> > > may read some secret data into cache.
> > >
> > > and then the attacker can use side-channel attack to find out what the
> > > secret data is.
> >
> > This code works after check_prlimit_permission call, which means you already
> > should have a permission granted. And you implies that misprediction gonna
> > be that deep which involves a number of calls/read/writes/jumps/locks-rb-wb-flushes
> > and a bunch or other instructions, moreover all conditions are "mispredicted".
> > This is very bold and actually unproved claim!
> >
> > Note that I pointed the patch is fine in cleanup context but seriously I
> > don't see how this all can be exploitable in sense of spectre.
> 

	Cyrill

[PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

[Dianzhang Chen]

The `resource` in do_prlimit() is controlled by userspace via syscall: setrlimit(defined in kernel/sys.c), hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
The relevant code in do_prlimit() is as below：

if (resource >= RLIM_NLIMITS)
        return -EINVAL;
...
rlim = tsk->signal->rlim + resource;    // use resource as index
...
            *old_rlim = *rlim;

Fix this by sanitizing resource before using it to index tsk->signal->rlim.

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

diff --git a/kernel/sys.c b/kernel/sys.c
index bdbfe8d..7eba1ca 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1532,6 +1532,8 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
 
 	if (resource >= RLIM_NLIMITS)
 		return -EINVAL;
+
+	resource = array_index_nospec(resource, RLIM_NLIMITS);
 	if (new_rlim) {
 		if (new_rlim->rlim_cur > new_rlim->rlim_max)
 			return -EINVAL;
-- 
2.7.4

Re: [PATCH] kernel/sys.c: fix possible spectre-v1 in do_prlimit()

[Cyrill Gorcunov]

On Mon, May 27, 2019 at 03:23:08PM +0800, Dianzhang Chen wrote:
> The `resource` in do_prlimit() is controlled by userspace via syscall: setrlimit(defined in kernel/sys.c), hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
> The relevant code in do_prlimit() is as below：
> 
> if (resource >= RLIM_NLIMITS)
>         return -EINVAL;
> ...
> rlim = tsk->signal->rlim + resource;    // use resource as index
> ...
>             *old_rlim = *rlim;
> 
> Fix this by sanitizing resource before using it to index tsk->signal->rlim.
> 
> Signed-off-by: Dianzhang Chen <dianzhangchen0@gmail.com>
> ---
>  kernel/sys.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index bdbfe8d..7eba1ca 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1532,6 +1532,8 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
>  
>  	if (resource >= RLIM_NLIMITS)
>  		return -EINVAL;
> +
> +	resource = array_index_nospec(resource, RLIM_NLIMITS);
>  	if (new_rlim) {
>  		if (new_rlim->rlim_cur > new_rlim->rlim_max)
>  			return -EINVAL;

Could you please explain in details how array_index_nospec is different
from resource >= RLIM_NLIMITS? Since I don't get how it is related to
spectre issue.