Re: [PATCH v4 07/39] x86: Add user control-protection fault handler

[Edgecombe, Rick P]

On Wed, 2022-12-21 at 11:41 +0100, Borislav Petkov wrote:
> On Wed, Dec 21, 2022 at 12:37:51AM +0000, Edgecombe, Rick P wrote:
> > You mean having separate paths for kernel IBT and user shadow stack
> > that compile out? I guess it could just all be in place if
> > CONFIG_X86_CET is in place.
> > 
> > I don't know, I thought it was relatively clean, but I can remove
> > it.
> 
> Yeah, I'm wondering if we really need the ifdeffery. I always
> question
> ifdeffery because it is a) ugly, b) a mess to deal with and having it
> is
> not really worth it. Yeah, we save a couple of KBs, big deal.
> 
> What would practically happen is, shadow stack will be default-
> enabled
> on the majority of kernels out there - distro ones - so it will be
> enabled practically everywhere.
> 
> And it'll be off only in some self-built kernels which are the very
> small minority.
> 
> And how much are the space savings with the whole set applied, with
> and
> without the Kconfig item enabled? Probably only a couple of KBs.

Oh, you mean the whole Kconfig thing. Yea, I mean I see the point about
typical configs. But at least CONFIG_X86_CET seems consistent with
CONFIG_INTEL_TDX_GUEST, CONFIG_IOMMU_SVA, etc.

What about moving it out of traps.c to a cet.c, like
exc_vmm_communication for CONFIG_AMD_MEM_ENCRT? Then the inclusion
logic lives in the build files, instead of an ifdef.

> 
> And if so, I'm thinking we could at least make the traps.c stuff
> unconditional - it'll be there but won't run. Unless we get some
> weird
> #CP but it'll be caught by do_unexpected_cp().
> 
> And you have feature tests everywhere so it's not like it'll get
> "misused".
> 
> And when you do that, you'll have everything a lot simpler, a lot
> less
> Kconfig items to build-test and all good.
> 
> Right?
> 
> Or am I completely way off into the weeds here and am missing an
> important aspect...?
> 

One aspect that has come up a couple of times, is how closely related
all these CET features are (or aren't). Shadow stack and IBT are mostly
separate, but do share an xfeature and an exception type. Similarly for
supervisor and user mode support for either of the CET features. So
maybe that is what is unusual here. There are some aspects that make
them look like separate features, which leads people to think they
should be separate in the code. But actually separating them leads to
excess ifdefery.

To me, putting the whole handler in even if there are no CET features
seems like too much. Leaving it in unconditionally is apparently also
inconsistent with some of the previous traps.c decisions. So I'd leave
CONFIG_X86_CET at least and it can help anyone building super stripped
down kernels. But I'm ok with whatever people think.