On 24/10/2024 12:59, Catalin Marinas wrote:
> On Wed, Oct 23, 2024 at 04:05:09PM +0100, Kevin Brodsky wrote:
>> +/*
>> + * Save the unpriv access state into ua_state and reset it to disable any
>> + * restrictions.
>> + */
>> +static void save_reset_user_access_state(struct user_access_state *ua_state)
>> +{
>> + if (system_supports_poe()) {
>> + /*
>> + * Enable all permissions in all 8 keys
>> + * (inspired by REPEAT_BYTE())
>> + */
>> + u64 por_enable_all = (~0u / POE_MASK) * POE_RXW;
> I think this should be ~0ul.
It is ~0u on purpose, because unlike in REPEAT_BYTE(), I only wanted the
lower 32 bits to be filled with POE_RXW (we only have 8 keys, the top 32
bits are RES0). That said, given that D128 has 4-bit pkeys, we could
anticipate and fill the top 32 bits too (should make no difference on D64).
>> @@ -907,6 +964,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
>> {
>> struct pt_regs *regs = current_pt_regs();
>> struct rt_sigframe __user *frame;
>> + struct user_access_state ua_state;
>>
>> /* Always make any pending restarted system calls return -EINTR */
>> current->restart_block.fn = do_no_restart_syscall;
>> @@ -923,12 +981,14 @@ SYSCALL_DEFINE0(rt_sigreturn)
>> if (!access_ok(frame, sizeof (*frame)))
>> goto badframe;
>>
>> - if (restore_sigframe(regs, frame))
>> + if (restore_sigframe(regs, frame, &ua_state))
>> goto badframe;
>>
>> if (restore_altstack(&frame->uc.uc_stack))
>> goto badframe;
>>
>> + restore_user_access_state(&ua_state);
>> +
>> return regs->regs[0];
>>
>> badframe:
> The saving part I'm fine with. For restoring, I was wondering whether we
> can get a more privileged POR_EL0 if reading the frame somehow failed.
> This is largely theoretical, there are other ways to attack like
> writing POR_EL0 directly than unmapping/remapping the signal stack.
>
> What I'd change here is always restore_user_access_state() to
> POR_EL0_INIT. Maybe just initialise ua_state above and add the function
> call after the badframe label.
I'm not sure I understand. When we enter this function, POR_EL0 is set
to whatever the signal handler set it to (POR_EL0_INIT by default).
There are then two cases:
1) Everything succeeds, including reading the saved POR_EL0 from the
frame. We then call restore_user_access_state(), setting POR_EL0 to the
value we've read, and return to userspace.
2) Any uaccess fails (for instance reading POR_EL0). In that case we
leave POR_EL0 unchanged and deliver SIGSEGV.
In case 2 POR_EL0 is most likely already set to POR_EL0_INIT, or
whatever the signal handler set it to. It's not clear to me that forcing
it to POR_EL0_INIT helps much. Either way it's doubtful that the SIGSEGV
handler will be able to recover, since the new signal frame we will
create for it may be a mix of interrupted state and signal handler state
(depending on exactly where we fail).
Kevin
