| Message ID | 20200128184934.77625-10-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for Clang's Shadow Call Stack | expand |
On Tue, Jan 28, 2020 at 10:49:32AM -0800, Sami Tolvanen wrote: > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > different exception level. > > Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > --- > arch/arm64/kvm/hyp/Makefile | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index ea710f674cb6..5843adef9ef6 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -28,3 +28,6 @@ GCOV_PROFILE := n > KASAN_SANITIZE := n > UBSAN_SANITIZE := n > KCOV_INSTRUMENT := n > + > +# remove the SCS flags from all objects in this directory > +KBUILD_CFLAGS := $(filter-out -ffixed-x18 $(CC_FLAGS_SCS), $(KBUILD_CFLAGS)) Acked-by: Will Deacon <will@kernel.org> Will
Hi Sami, On 28/01/2020 18:49, Sami Tolvanen wrote: > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > different exception level. Hmmm, there are two things being disabled here. Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but shouldn't KVM's C code still treat x18 as a fixed register? As you have an __attribute__((no_sanitize("shadow-call-stack"))), could we add that to __hyp_text instead? (its a smaller hammer!) All of KVM's EL2 code is marked __hyp_text, but that isn't everything in these files. Doing it like this would leave KVM's VHE-only paths covered. As an example, with VHE the kernel and KVM both run at EL2, and KVM behaves differently: kvm_vcpu_put_sysregs() in kvm/hyp/sysreg-sr.c is called from a preempt notifier as the EL2 registers are always accessible. Thanks, James > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index ea710f674cb6..5843adef9ef6 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -28,3 +28,6 @@ GCOV_PROFILE := n > KASAN_SANITIZE := n > UBSAN_SANITIZE := n > KCOV_INSTRUMENT := n > + > +# remove the SCS flags from all objects in this directory > +KBUILD_CFLAGS := $(filter-out -ffixed-x18 $(CC_FLAGS_SCS), $(KBUILD_CFLAGS))
On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: > On 28/01/2020 18:49, Sami Tolvanen wrote: > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > > different exception level. > > Hmmm, there are two things being disabled here. > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but > shouldn't KVM's C code still treat x18 as a fixed register? My review of v6 suggested dropping the -ffixed-x18 as well, since it's only introduced by SCS (in patch 5) and so isn't required by anything else. Why do you think it's needed? > As you have an __attribute__((no_sanitize("shadow-call-stack"))), could we add that to > __hyp_text instead? (its a smaller hammer!) All of KVM's EL2 code is marked __hyp_text, > but that isn't everything in these files. Doing it like this would leave KVM's VHE-only > paths covered. > > As an example, with VHE the kernel and KVM both run at EL2, and KVM behaves differently: > kvm_vcpu_put_sysregs() in kvm/hyp/sysreg-sr.c is called from a preempt notifier as > the EL2 registers are always accessible. That's a good point, and I agree that it would be nice to have SCS covering the VHE paths. If you do that as a function attribute (which feels pretty fragile to me), then I guess we'll have to keep the -ffixed-x18 for the non-VHE code after all because GCC at least doesn't like having the register saving ABI specified on a per-function basis. Will
On Mon, Feb 10, 2020 at 05:52:15PM +0000, Will Deacon wrote: > On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: > > On 28/01/2020 18:49, Sami Tolvanen wrote: > > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > > > different exception level. > > > > Hmmm, there are two things being disabled here. > > > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but > > shouldn't KVM's C code still treat x18 as a fixed register? > > My review of v6 suggested dropping the -ffixed-x18 as well, since it's only > introduced by SCS (in patch 5) and so isn't required by anything else. Why > do you think it's needed? When EL1 code calls up to hyp, it expects x18 to be preserved across the call, so hyp needs to either preserve it explicitly across a transitions from/to EL1 or always preserve it. The latter is easiest since any code used by VHE hyp code will need x18 saved anyway (ans so any common hyp code needs to). Thanks, Mark.
On Mon, Feb 10, 2020 at 06:03:28PM +0000, Mark Rutland wrote: > On Mon, Feb 10, 2020 at 05:52:15PM +0000, Will Deacon wrote: > > On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: > > > On 28/01/2020 18:49, Sami Tolvanen wrote: > > > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > > > > different exception level. > > > > > > Hmmm, there are two things being disabled here. > > > > > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but > > > shouldn't KVM's C code still treat x18 as a fixed register? > > > > My review of v6 suggested dropping the -ffixed-x18 as well, since it's only > > introduced by SCS (in patch 5) and so isn't required by anything else. Why > > do you think it's needed? > > When EL1 code calls up to hyp, it expects x18 to be preserved across the > call, so hyp needs to either preserve it explicitly across a transitions > from/to EL1 or always preserve it. I thought we explicitly saved/restored it across the call after af12376814a5 ("arm64: kvm: stop treating register x18 as caller save"). Is that not sufficient? > The latter is easiest since any code used by VHE hyp code will need x18 > saved anyway (ans so any common hyp code needs to). I would personally prefer to split the VHE and non-VHE code so they can be compiled with separate options. Will
On Mon, Feb 10, 2020 at 06:07:41PM +0000, Will Deacon wrote: > On Mon, Feb 10, 2020 at 06:03:28PM +0000, Mark Rutland wrote: > > On Mon, Feb 10, 2020 at 05:52:15PM +0000, Will Deacon wrote: > > > On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: > > > > On 28/01/2020 18:49, Sami Tolvanen wrote: > > > > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > > > > > different exception level. > > > > > > > > Hmmm, there are two things being disabled here. > > > > > > > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but > > > > shouldn't KVM's C code still treat x18 as a fixed register? > > > > > > My review of v6 suggested dropping the -ffixed-x18 as well, since it's only > > > introduced by SCS (in patch 5) and so isn't required by anything else. Why > > > do you think it's needed? > > > > When EL1 code calls up to hyp, it expects x18 to be preserved across the > > call, so hyp needs to either preserve it explicitly across a transitions > > from/to EL1 or always preserve it. > > I thought we explicitly saved/restored it across the call after > af12376814a5 ("arm64: kvm: stop treating register x18 as caller save"). Is > that not sufficient? That covers the hyp->guest->hyp round trip, but not the host->hyp->host portion surrounding that. Anywhere we use __call_hyp() expects x18 to be preserved across the call, and that's not only used to enter the guest. If we don't want to do that naturally at EL2, we'd probably have to add that to do_el2_call in arch/arm64/kvm/hyp/hyp-entry.S. Thanks, Mark.
On 2020-02-10 18:07, Will Deacon wrote: > On Mon, Feb 10, 2020 at 06:03:28PM +0000, Mark Rutland wrote: >> On Mon, Feb 10, 2020 at 05:52:15PM +0000, Will Deacon wrote: >> > On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: >> > > On 28/01/2020 18:49, Sami Tolvanen wrote: >> > > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a >> > > > different exception level. >> > > >> > > Hmmm, there are two things being disabled here. >> > > >> > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but >> > > shouldn't KVM's C code still treat x18 as a fixed register? >> > >> > My review of v6 suggested dropping the -ffixed-x18 as well, since it's only >> > introduced by SCS (in patch 5) and so isn't required by anything else. Why >> > do you think it's needed? >> >> When EL1 code calls up to hyp, it expects x18 to be preserved across >> the >> call, so hyp needs to either preserve it explicitly across a >> transitions >> from/to EL1 or always preserve it. > > I thought we explicitly saved/restored it across the call after > af12376814a5 ("arm64: kvm: stop treating register x18 as caller save"). > Is > that not sufficient? > >> The latter is easiest since any code used by VHE hyp code will need >> x18 >> saved anyway (ans so any common hyp code needs to). > > I would personally prefer to split the VHE and non-VHE code so they can > be > compiled with separate options. This is going to generate a lot of code duplication (or at least object duplication), as the two code paths are intricately linked and splitting them to support different compilation options and/or calling conventions. I'm not fundamentally opposed to that, but it should come with ways to still manage it as a unified code base as much as possible, as ways to discard the unused part at runtime (which should become easy to do once we have two distinct sets of objects). M.
On Mon, Feb 10, 2020 at 06:24:32PM +0000, Mark Rutland wrote: > On Mon, Feb 10, 2020 at 06:07:41PM +0000, Will Deacon wrote: > > On Mon, Feb 10, 2020 at 06:03:28PM +0000, Mark Rutland wrote: > > > On Mon, Feb 10, 2020 at 05:52:15PM +0000, Will Deacon wrote: > > > > On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: > > > > > On 28/01/2020 18:49, Sami Tolvanen wrote: > > > > > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > > > > > > different exception level. > > > > > > > > > > Hmmm, there are two things being disabled here. > > > > > > > > > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but > > > > > shouldn't KVM's C code still treat x18 as a fixed register? > > > > > > > > My review of v6 suggested dropping the -ffixed-x18 as well, since it's only > > > > introduced by SCS (in patch 5) and so isn't required by anything else. Why > > > > do you think it's needed? > > > > > > When EL1 code calls up to hyp, it expects x18 to be preserved across the > > > call, so hyp needs to either preserve it explicitly across a transitions > > > from/to EL1 or always preserve it. > > > > I thought we explicitly saved/restored it across the call after > > af12376814a5 ("arm64: kvm: stop treating register x18 as caller save"). Is > > that not sufficient? > > That covers the hyp->guest->hyp round trip, but not the host->hyp->host > portion surrounding that. Thanks, I missed that. It's annoying that we'll end up needing /both/ -ffixed-x18 *and* the save/restore around guest transitions, but if we actually want to use SCS for the VHE code then I see that it will be required. Sami -- can you restore -ffixed-x18 and then try the function attribute as suggested by James, please? Will
On Tue, Feb 11, 2020 at 09:14:52AM +0000, Marc Zyngier wrote: > On 2020-02-10 18:07, Will Deacon wrote: > > On Mon, Feb 10, 2020 at 06:03:28PM +0000, Mark Rutland wrote: > > > On Mon, Feb 10, 2020 at 05:52:15PM +0000, Will Deacon wrote: > > > > On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: > > > > > On 28/01/2020 18:49, Sami Tolvanen wrote: > > > > > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a > > > > > > different exception level. > > > > > > > > > > Hmmm, there are two things being disabled here. > > > > > > > > > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but > > > > > shouldn't KVM's C code still treat x18 as a fixed register? > > > > > > > > My review of v6 suggested dropping the -ffixed-x18 as well, since it's only > > > > introduced by SCS (in patch 5) and so isn't required by anything else. Why > > > > do you think it's needed? > > > > > > When EL1 code calls up to hyp, it expects x18 to be preserved across > > > the > > > call, so hyp needs to either preserve it explicitly across a > > > transitions > > > from/to EL1 or always preserve it. > > > > I thought we explicitly saved/restored it across the call after > > af12376814a5 ("arm64: kvm: stop treating register x18 as caller save"). > > Is > > that not sufficient? > > > > > The latter is easiest since any code used by VHE hyp code will need > > > x18 > > > saved anyway (ans so any common hyp code needs to). > > > > I would personally prefer to split the VHE and non-VHE code so they can > > be > > compiled with separate options. > > This is going to generate a lot of code duplication (or at least object > duplication), > as the two code paths are intricately linked and splitting them to support > different > compilation options and/or calling conventions. > > I'm not fundamentally opposed to that, but it should come with ways to still > manage it as a unified code base as much as possible, as ways to discard the > unused part at runtime (which should become easy to do once we have two > distinct sets of objects). Agreed, and I don't want to hold up the SCS patches because of this. I'm just nervous about the function attribute because I've only ever had terrible experiences with them. Maybe it will work this time (!) Will
On 2020-02-11 09:55, Will Deacon wrote: > On Tue, Feb 11, 2020 at 09:14:52AM +0000, Marc Zyngier wrote: >> On 2020-02-10 18:07, Will Deacon wrote: >> > On Mon, Feb 10, 2020 at 06:03:28PM +0000, Mark Rutland wrote: >> > > On Mon, Feb 10, 2020 at 05:52:15PM +0000, Will Deacon wrote: >> > > > On Mon, Feb 10, 2020 at 05:18:58PM +0000, James Morse wrote: >> > > > > On 28/01/2020 18:49, Sami Tolvanen wrote: >> > > > > > Filter out CC_FLAGS_SCS and -ffixed-x18 for code that runs at a >> > > > > > different exception level. >> > > > > >> > > > > Hmmm, there are two things being disabled here. >> > > > > >> > > > > Stashing the lr in memory pointed to by VA won't work transparently at EL2 ... but >> > > > > shouldn't KVM's C code still treat x18 as a fixed register? >> > > > >> > > > My review of v6 suggested dropping the -ffixed-x18 as well, since it's only >> > > > introduced by SCS (in patch 5) and so isn't required by anything else. Why >> > > > do you think it's needed? >> > > >> > > When EL1 code calls up to hyp, it expects x18 to be preserved across >> > > the >> > > call, so hyp needs to either preserve it explicitly across a >> > > transitions >> > > from/to EL1 or always preserve it. >> > >> > I thought we explicitly saved/restored it across the call after >> > af12376814a5 ("arm64: kvm: stop treating register x18 as caller save"). >> > Is >> > that not sufficient? >> > >> > > The latter is easiest since any code used by VHE hyp code will need >> > > x18 >> > > saved anyway (ans so any common hyp code needs to). >> > >> > I would personally prefer to split the VHE and non-VHE code so they can >> > be >> > compiled with separate options. >> >> This is going to generate a lot of code duplication (or at least >> object >> duplication), >> as the two code paths are intricately linked and splitting them to >> support >> different >> compilation options and/or calling conventions. >> >> I'm not fundamentally opposed to that, but it should come with ways to >> still >> manage it as a unified code base as much as possible, as ways to >> discard the >> unused part at runtime (which should become easy to do once we have >> two >> distinct sets of objects). > > Agreed, and I don't want to hold up the SCS patches because of this. > I'm > just nervous about the function attribute because I've only ever had > terrible experiences with them. Maybe it will work this time (!) I have the same experience chasing missing __hyp_text attributes. Until we have tooling that picks on this *at compile time*, we'll have to play wack-a-mole with them... M.
On Tue, Feb 11, 2020 at 1:54 AM Will Deacon <will@kernel.org> wrote: > Thanks, I missed that. It's annoying that we'll end up needing /both/ > -ffixed-x18 *and* the save/restore around guest transitions, but if we > actually want to use SCS for the VHE code then I see that it will be > required. > > Sami -- can you restore -ffixed-x18 and then try the function attribute > as suggested by James, please? Sure. Adding __noscs to __hyp_text and not filtering out any of the flags in the Makefile appears to work. I'll update this in the next version. Sami
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile index ea710f674cb6..5843adef9ef6 100644 --- a/arch/arm64/kvm/hyp/Makefile +++ b/arch/arm64/kvm/hyp/Makefile @@ -28,3 +28,6 @@ GCOV_PROFILE := n KASAN_SANITIZE := n UBSAN_SANITIZE := n KCOV_INSTRUMENT := n + +# remove the SCS flags from all objects in this directory +KBUILD_CFLAGS := $(filter-out -ffixed-x18 $(CC_FLAGS_SCS), $(KBUILD_CFLAGS))