Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}
Date: Mon, 16 Aug 2021 11:31:51 +0100	[thread overview]
Message-ID: <1c1f92bd-f8de-d21c-6dfa-2bb811f7b814@citrix.com> (raw)
In-Reply-To: <94239a81-1d8a-e6ec-f572-9c8d5f3ec50f@suse.com>

On 13/08/2021 15:00, Jan Beulich wrote:
> On 12.08.2021 19:03, Andrew Cooper wrote:
>> This was a clear oversight in the original CET work.  The BUGFRAME_run_fn and
>> BUGFRAME_warn paths update regs->rip without an equivlenet adjustment to the
>> shadow stack, causes IRET to suffer #CP due to the mismatch.
>>
>> One subtle, and therefore fragile, aspect of extable_shstk_fixup() was that it
>> required regs->rip to have its old value as a cross-check that the correct
>> word in the shadow stack was being adjusted.
>>
>> Rework extable_shstk_fixup() into fixup_exception_return() which takes
>> ownership of the update to both the regular and shadow stacks, ensuring that
>> the regs->rip update is ordered suitably.
>>
>> Use the new fixup_exception_return() for BUGFRAME_run_fn and BUGFRAME_warn to
>> ensure that the shadow stack is updated too.
>>
>> Fixes: 209fb9919b50 ("x86/extable: Adjust extable handling to be shadow stack compatible")
>> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> I'm not a massive fan of the large ifdef area.  The logic could be rearranged
>> to use IS_ENABLED(CONFIG_XEN_SHSTK) by indenting most of the function, but I
>> can't see any way to drop the goto's, and this is certainly the least-invasive
>> diff.
> It's not really neat, but we've got worse code elsewhere.
>
> I wonder whether gdb_arch_resume() and gdb_arch_write_reg() also
> need some sort of similar adjustment.

Hmm.

So there's nothing we can do right now, because GDB has yet to gain an
understanding of shadow stacks (or rather, the prototype so far is still
under discussion on LKML, given that CET support has yet to be included
into Linux).

Beyond that, I haven't seen gdbstub modified in a decade, nor have I
found anyone who's used it.  I'd be astounded if it actually works.

I think its fine for people using GDB to know that they need to turn off
CET first, but I don't expect anyone to have a pleasant time trying to
use gdbstub to begin with in Xen.

~Andrew