Re: [PATCH v2 3/4] arm64/entry-common: Make Aarch32 exceptions' availability depend on aarch32_enabled()

[Andrea della Porta]

On 12:27 Wed 25 Oct     , Mark Rutland wrote:
> On Mon, Oct 23, 2023 at 04:42:22PM +0200, Andrea della Porta wrote:
> > Another major aspect of supporting running of 32bit processes is the
> > ability to access 32bit syscalls and exceptions. Syscalls, in
> > particular, can be invoked by using the svc instruction.
> > 
> > If Aarch32 support is disabled ensure that calling svc (or any
> > exceptions) from 32bit context results in the same behavior as if
> > CONFIG_COMPAT has not been enabled (i.e. a kernel panic).
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@xxxxxxxx>
> 
> Just to be clear, as it stands, I don't think we should apply this patch.
> 
> * There's no justficiation so far for disabling the vectors given they should
>   be unreachable.
>

True, but let's see what it buys and what not:

- is it really necessary to check for 32 bit enablement when you cannot run 
32 bit excecutable (from binfmt loader) in teh first place? Obviously not at all,
unless you find a way (of which I'm not aware right now) to switch to 32 bit mode
maybe by expliting some execpotion return path. In that (admittedly unlikely, as
of now) case, you would expose all 32 bit syscall to be used by userspace.

- does 'redundantly' checking for 32 bit alignment in the vector handler pose
some performence bottleneck? Yes, but only in the dynamic enabled case (i.e.
you have enabled the 32 bit support via the proposed kernel parameter). 
All other cases are compiler optimized away or simply not reachable under normal
circumstances, so it's redundant only in very few (and probably irrelevant) 
cases. 
 
> * If we really want to do this, the behaviour should be driven by a cpucap, so
>   as to have truly negligible impact and to be consistent with how we handle
>   features elsewhere.
> 
>   That will require some preparatory rework to the way de handle detecing
>   support for AArch32 at EL0 (some of which I think we should do anyway as a
>   cleanup).
> 
> * We should not introduce new *_ni_handler() functions. If we really want to
>   do this, we should refactor the existing code such that we have a single
>   el0t_32_<vector>_handler() implementation for each vector regardless of
>   CONFIG_COMPAT, which can figure out what to do, e.g. something like:
> 
>   | asmlinkage void noinstr el0t_32_irq_handler(struct pt_regs *regs)
>   | {
>   |         if (!system_suports_compat_el0())
>   | 	            panic_unhandled_vector(el0t, 32, irq, regs);
>   |             
>   | 	    __el0_irq_handler_common(regs);
>   | }
> 
>   That way the feature check only needs to test IS_ENABLED(CONFIG_COMPAT) and
>   alternative_has_cap(whatever), and we can rely on the compiler to elide the
>   unreachable code. That way we use the exact same code for the case that
>   32-bit support is disabled statically or dynamically.
> 
>   I had a quick go at mocking that up:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/entry/unhandled-rework
> 
>   ... which doesn't look too hard.
>  

I agree with that, so assuming we still want to proceed and elaborate further,
I'd like to sort the various proposals since I'm a little bit confused about
them: from one side you propose to rework the vector handler definitions and
to insert the conditional logic inside them. On the other side there is the 
thread from you, Kees and others that eventually led to a seccomp assisted
solution as the preferred way. I'm not entirely sure how seccomp can be used here,
whether setting up a bpf filter from inside the kernel (that enable or disable
the 32 bit handler based on the kernel parameter) or from userspace (systemd,
others..), so I assume we're still interested to investigate on your proposed solution. 
Regarding that, I propose the following:

- rework the vector handler definitions as you've already mocked up, in
order to get rid of the UNHANDLED declarations.

- use the actual system_supports_32bit_el0() call as the conditional inside
the vector handler. That routine in turn is calling id_aa64pfr0_32bit_el0()
that should load the ID_AA64PFR0_EL1. We can override the EL0 nibble by 
leveraging the idreg-override machinery, as noted by robin.murphy. In this
way we don't even need to add a new kernel parameter, maybe just a user friendly
mnemonic alias to that register, such like 'arm64.no32bit-el0'.

- CONFIG_COMPAT can be checked in the vector handler as you proposed in your
mock up or maybe event inside system_supports_32bit_el0() - need to check
whether the compiler is able to optimize away the static case even in that
case.

Does it sounds reasonable?

Many thanks,
Andrea