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

[Dave Martin]

On Mon, Oct 21, 2024 at 12:06:07PM +0200, Kevin Brodsky wrote:
> 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.

"user_access" seemed natural to me: it parses equally as "[user
access]" (i.e., uaccess) and "[user] access" (i.e., access by, to, or
on behalf of user(space)).

Introducing an architectural term when there is already a generic OS
and Linux kernel term that means the right thing seemed not to improve
readability, but I guess it's a matter of opinion.

Anyway, it doesn't really matter.

> 
> > 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;
> > [...]

^

Ah, good point.  The save_reset_unpriv_access_state(&ua_state) call
probably belong just before the first __put_user() then.

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

Certainly if that struct grows more state, then the factoring will
help in future.  I wasn't clear on how we expect this all to evolve.

Either way, this is basically a non-issue, and keeping the symmetry is
probably a good idea.

Cheers
---Dave