Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200717110532.238533-1-asteinhauser@google.com> <20200907171825.GB13281@willie-the-truck>
In-Reply-To: <20200907171825.GB13281@willie-the-truck>
From:   Anthony Steinhauser <asteinhauser@google.com>
Date:   Sun, 20 Sep 2020 03:25:23 -0700
Message-ID: <CAN_oZf29J-DMF04EhQf=29CzV5zrPT+L6HchZcG-CDt=MjFf4A@mail.gmail.com>
Subject: Re: [PATCH] PR_SPEC_DISABLE_NOEXEC support for arm64.
To:     Will Deacon <will@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, catalin.marinas@arm.com,
        maz@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> As a heads up: I'm currently reworking most of this, and hope to post
> something within the next two weeks.

Sure. Let me know whether you want to implement the
PR_SPEC_DISABLE_NOEXEC support directly or whether this patch would be
relevant even after your rework.
>
> > diff --git a/arch/arm64/include/asm/ssbd.h b/arch/arm64/include/asm/ssbd.h
> > new file mode 100644
> > index 000000000000..68c716dc5811
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/ssbd.h
...
> > +}
>
> I'd prefer to keep these where they are and have an out-of-line call if
> necessary. We should try to keep the SSBD stuff in one place.

OK.
>
> > +
> > +#endif       /* __ASM_SSBD_H */
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 6089638c7d43..ad3c67c86c4c 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -54,6 +54,7 @@
> >  #include <asm/mmu_context.h>
> >  #include <asm/processor.h>
> >  #include <asm/pointer_auth.h>
> > +#include <asm/ssbd.h>
> >  #include <asm/stacktrace.h>
> >
> >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > @@ -588,6 +589,18 @@ void arch_setup_new_exec(void)
> >       current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
> >
> >       ptrauth_thread_init_user(current);
> > +
> > +     /*
> > +      * Don't inherit TIF_SSBD across exec boundary when
> > +      * PR_SPEC_DISABLE_NOEXEC is used.
> > +      */
> > +     if (test_thread_flag(TIF_SSBD) &&
> > +         task_spec_ssb_noexec(current)) {
> > +             clear_thread_flag(TIF_SSBD);
> > +             task_clear_spec_ssb_disable(current);
> > +             task_clear_spec_ssb_noexec(current);
> > +             ssbd_ssbs_enable(current);
> > +     }
>
> How is this supposed to work with CPUs that expose SSBS directly to
> userspace? I suppose we should be using PR_SPEC_DISABLE_NOEXEC to decide
> what we set the SSBS bit to on exec, but the logic here requires TIF_SSBD
> to be set and so won't trigger afaict.
>

You're right. The SSBS support is incomplete. I guess
"test_thread_flag(TIF_SSBD)" can be replaced just with
"arm64_get_ssbd_state() == ARM64_SSBD_KERNEL".
Thanks,
Anthony