linux-kernel - Re: [PATCH] sched/scs: Reset the shadow stack when idle_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHn4Dec0Jyc30vWMLAXwQ-ge4eS5S26hxfMky-e4f-TTtFrbEQ@mail.gmail.com>
Date:   Tue, 12 Oct 2021 18:35:48 +0800
From:   Woody Lin <woodylin@...gle.com>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     Ingo Molnar <mingo@...hat.com>, Ben Segall <bsegall@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/scs: Reset the shadow stack when idle_task_exit

On Tue, Oct 12, 2021 at 6:00 PM Valentin Schneider
<valentin.schneider@....com> wrote:
>
> On 12/10/21 16:35, Woody Lin wrote:
> > There was a 'init_idle' that resets scs sp to base, but is removed by
> > f1a0a376ca0c. Without the resetting, the hot-plugging implemented by
> > cpu_psci_cpu_boot will use the previous scs sp as new base when starting
> > up a CPU core, so the usage on scs page is being stacked up until
> > overflow.
> >
> > This only happens on idle task since __cpu_up is using idle task as the
> > main thread to start up a CPU core, so the overflow can be fixed by
> > resetting scs sp to base in 'idle_task_exit'.
> >
>
> Looking at init_idle() for similar issues, it looks like we might also want
> to re-issue kasan_unpoison_task_stack() on the idle task upon hotplug.
>
> > Fixes: f1a0a376ca0c ("sched/core: Initialize the idle task with preemption disabled")
> > Signed-off-by: Woody Lin <woodylin@...gle.com>
> > ---
> >  kernel/sched/core.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 1bba4128a3e6..f21714ea3db8 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8795,6 +8795,7 @@ void idle_task_exit(void)
> >               finish_arch_post_lock_switch();
> >       }
> >
> > +     scs_task_reset(current);
> >       /* finish_cpu(), as ran on the BP, will clean up the active_mm state */
>
> So AIUI for SCS that works just fine - one thing I'm unclear on is how the
> following pops are going to work given the SP reset happens in the middle
> of a call stack, but AFAICT that was already the case before I messed about
> with init_idle(), so that must already be handled.

Hi Valentin,

Thanks for the question. The 'scs_task_reset' here resets only the
'.thread_info.scs_sp' of the task, so the register (on arm64 it's x18)
is still pointing to the same location for popping and storing call
frames. The register will be updated to '.thread_info.scs_sp' in
'__secondary_switched', which starts a new core and there is no popping
after the updating, so it won't introduce an underflow.

>
> I'm not familiar enough with KASAN to say whether that
> kasan_unpoison_task_stack() should rather happen upon hotplugging the CPU
> back (rather than on hot-unplug). If that is the case, then maybe somewhere
> around cpu_startup_entry() might work (and then you could bunch these two
> "needs to be re-run at init for the idle task" functions into a common
> helper).

unpoison looks more like an one-time thing to me; the idle tasks will
reuse the same stack pages until system resets, so I think we don't need
to re-unpoison that during hotplugging as long as it's unpoisoned in
'init_idle'.

>
> >  }
> >
> > --
> > 2.33.0.882.g93a45727a2-goog