Re: [PATCH 3/5] arm64: signal: Improve POR_EL0 handling to avoid uaccess failures

[Kevin Brodsky <kevin.brodsky@arm.com>]

On 17/10/2024 17:53, Dave Martin wrote:
> [...]
>> +/*
>> + * Save the unpriv access state into ua_state and reset it to disable any
>> + * restrictions.
>> + */
>> +static void save_reset_unpriv_access_state(struct unpriv_access_state *ua_state)
> Would _user_ be more consistent naming than _unpriv_ ?

I did ponder on the naming. I considered user_access/uaccess instead of
unpriv_access, but my concern is that it might imply that only uaccess
is concerned, while in reality loads/stores that userspace itself
executes are impacted too. I thought using the "unpriv" terminology from
the Arm ARM (used for stage 1 permissions) might avoid such
misunderstanding. I'm interested to hear opinions on this, maybe
accuracy sacrifices readability.

> Same elsewhere.
>
>> +{
>> +	if (system_supports_poe()) {
>> +		/*
>> +		 * Enable all permissions in all 8 keys
>> +		 * (inspired by REPEAT_BYTE())
>> +		 */
>> +		u64 por_enable_all = (~0u / POE_MASK) * POE_RXW;
> Yikes!
>
> Seriously though, why are we granting permissions that the signal
> handler isn't itself going to have over its own stack?
>
> I think the logical thing to do is to think of the write/read of the
> signal frame as being done on behalf of the signal handler, so the
> permissions should be those we're going to give the signal handler:
> not less, and (so far as we can approximate) not more.

Will continue that discussion on the cover letter.

>
>> +
>> +		ua_state->por_el0 = read_sysreg_s(SYS_POR_EL0);
>> +		write_sysreg_s(por_enable_all, SYS_POR_EL0);
>> +		/* Ensure that any subsequent uaccess observes the updated value */
>> +		isb();
>> +	}
>> +}
>> +
>> +/*
>> + * Set the unpriv access state for invoking the signal handler.
>> + *
>> + * No uaccess should be done after that function is called.
>> + */
>> +static void set_handler_unpriv_access_state(void)
>> +{
>> +	if (system_supports_poe())
>> +		write_sysreg_s(POR_EL0_INIT, SYS_POR_EL0);
>> +
> Spurious blank line?

Thanks!

>> +}
> [...]
>
>> @@ -1252,9 +1310,11 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>>  {
>>  	struct rt_sigframe_user_layout user;
>>  	struct rt_sigframe __user *frame;
>> +	struct unpriv_access_state ua_state;
>>  	int err = 0;
>>  
>>  	fpsimd_signal_preserve_current_state();
>> +	save_reset_unpriv_access_state(&ua_state);
> (Trivial nit: maybe put the blank line before this rather than after?
> This has nothing to do with "settling" the kernel's internal context
> switch state, and a lot to do with generaing the signal frame...)

In fact considering the concern Catalin brought up with POR_EL0 being
reset even when we fail to deliver the signal [1], I'm realising this
call should be moved after get_sigframe(), since the latter doesn't use
uaccess and can fail.

[1] https://lore.kernel.org/linux-arm-kernel/Zw6D2waVyIwYE7wd@arm.com/

>>  
>>  	if (get_sigframe(&user, ksig, regs))
>>  		return 1;
> [...]
>
>> @@ -1273,6 +1333,7 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>>  			regs->regs[1] = (unsigned long)&frame->info;
>>  			regs->regs[2] = (unsigned long)&frame->uc;
>>  		}
>> +		set_handler_unpriv_access_state();
> This bit feels prematurely factored?  We don't have separate functions
> for the other low-level preparation done here...

I preferred to have a consistent API for all manipulations of POR_EL0,
the idea being that if more registers are added to struct
unpriv_access_state, only the *unpriv_access* helpers need to be amended.

> It works either way though, and I don't have a strong view.
>
> Overall, this all looks reasonable.

Thanks for the review!

Kevin