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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

On Mon, Aug 26, 2024 at 01:00:09PM +0300, Catalin Marinas wrote:
> 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:

> 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.

I think so, yes.

> If GCS is disabled for a guest, we save the GCSPR_EL0 with the cap size

s/guest/task/ I guess?

> 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've changed this so we instead only add the frame for the token if GCS
is enabled and updated the comment, that way we don't modify GCSPR_EL0
in cases where GCS is not enabled.

> 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.

I think the handling for the various options in the second case mean
that it's clearer and simpler to write once when we restore the frame
and once when we consume the token.

> 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.

I've moved gcs_restore_signal() before the altstack restore which I
think is what you're looking for here?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]