Re: [PATCH v11 25/39] arm64/signal: Expose GCS state in signal frames

[Catalin Marinas <catalin.marinas@arm.com>]

On Fri, Aug 23, 2024 at 11:01:13PM +0100, Mark Brown wrote:
> On Fri, Aug 23, 2024 at 04:59:11PM +0100, Catalin Marinas wrote:
> > On Fri, Aug 23, 2024 at 11:25:30AM +0100, Mark Brown wrote:
> 
> > > We could store either the cap token or the interrupted GCSPR_EL0 (the
> > > address below the cap token).  It felt more joined up to go with the cap
> > > token since notionally signal return is consuming the cap token but
> > > either way would work, we could just add an offset when looking at the
> > > pointer.
> 
> > In a hypothetical sigaltshadowstack() scenario, would the cap go on the
> > new signal shadow stack or on the old one? I assume on the new one but
> > in sigcontext we'd save the original GCSPR_EL0. In such hypothetical
> > case, the original GCSPR_EL0 would not need 8 subtracted.
> 
> I would have put the token on the old stack since that's what we'd be
> returning to.

After some more spec reading, your approach makes sense as it matches
the GCSSS[12] instructions where the outgoing, rather than incoming,
shadow stack is capped. So all good I think. However, a bit more below
on the restore order (it's ok but a bit confusing).

> This raises interesting questions about what happens if
> the reason for the signal is that we just overflowed the normal stack
> (which are among the issues that have got in the way of working out if
> or how we do something with sigaltshadowstack).

That's not that different from the classic case where we get an error
trying to setup the frame. signal_setup_done() handles it by forcing a
SIGSEGV. I'd say we do the same here.

> I'm not clear what the
> purpose of the token would be on the new stack, the token basically says
> "this is somewhere we can sigreturn to", that's not the case for the
> alternative stack.

Yeah, I thought we have to somehow mark the top of the stack with this
token. But looking at the architecture stack switching, it caps the
outgoing stack (in our case this would be the interrupted one). So
that's settled.

On the patch itself, I think there are some small inconsistencies on how
it reads the GCSPR_EL0: preserve_gcs_context() does a
gcs_preserve_current_state() and subsequently reads the value from the
thread structure. A bit later, gcs_signal_entry() goes for the sysreg
directly. I don't think that's a problem even if the thread gets
preempted but it would be nice to be consistent. Maybe leave the
gcs_preserve_current_state() only a context switch thing. Would it work
if we don't touch the thread structure at all in the signal code? We
wouldn't deliver a signal in the middle of the switch_to() code. So any
value we write in thread struct would be overridden at the next switch.

If GCS is disabled for a guest, we save the GCSPR_EL0 with the cap size
subtracted but there's no cap written. In restore_gcs_context() it
doesn't look like we add the cap size back when writing GCSPR_EL0. If
GCS is enabled, we do consume the cap and add 8 but otherwise it looks
that we keep decreasing GCSPR_EL0. I think we should always subtract the
cap size if GCS is enabled. This could could do with some refactoring as
I find it hard to follow (not sure exactly how, maybe just comments will
do).

I'd also keep a single write to GCSPR_EL0 on the return path but I'm ok
with two if we need to cope with GCS being disabled but the GCSPR_EL0
still being saved/restored.

Another aspect for gcs_restore_signal(), I think it makes more sense for
the cap to be consumed _after_ restoring the sigcontext since this has
the actual gcspr_el0 where we stored the cap and represents the original
stack. If we'll get an alternative shadow stack, current GCSPR_EL0 on
sigreturn points to that alternative shadow stack rather than the
original one. That's what confused me when reviewing the patch and I
thought the cap goes to the top of the signal stack.

-- 
Catalin