> On Aug 26, 2024, at 5:53 PM, Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
>
> Hi Aruna
>
> On Fri, Aug 9, 2024 at 10:30 AM Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
>>
>> On Thu, Aug 1, 2024 at 11:13 PM Aruna Ramakrishna
>> <aruna.ramakrishna@xxxxxxxxxx> wrote:
>>>
>>> If the alternate signal stack is protected by a different pkey than the
>>> current execution stack, copying xsave data to the sigaltstack will fail
>>> if its pkey is not enabled in the PKRU register.
>>>
>>> We do not know which pkey was used by the application for the altstack,
>>> so enable all pkeys before xsave.
>>>
>>> But this updated PKRU value is also pushed onto the sigframe, which
>>> means the register value restored from sigcontext will be different from
>>> the user-defined one, which is unexpected. Fix that by overwriting the
>>> PKRU value on the sigframe with the original, user-defined PKRU.
>>>
>>> Signed-off-by: Aruna Ramakrishna <aruna.ramakrishna@xxxxxxxxxx>
>>> ---
>>> arch/x86/kernel/fpu/signal.c | 11 +++++++++--
>>> arch/x86/kernel/signal.c | 12 ++++++++++--
>>> 2 files changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
>>> index 931c5469d7f3..1065ab995305 100644
>>> --- a/arch/x86/kernel/fpu/signal.c
>>> +++ b/arch/x86/kernel/fpu/signal.c
>>> @@ -168,8 +168,15 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
>>>
>>> static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf, u32 pkru)
>>> {
>>> - if (use_xsave())
>>> - return xsave_to_user_sigframe(buf);
>>> + int err = 0;
>>> +
>>> + if (use_xsave()) {
>>> + err = xsave_to_user_sigframe(buf);
>>> + if (!err)
>>> + err = update_pkru_in_sigframe(buf, pkru);
>>> + return err;
>>> + }
>>> +
>>> if (use_fxsr())
>>> return fxsave_to_user_sigframe((struct fxregs_state __user *) buf);
>>> else
>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>> index 9dc77ad03a0e..5f441039b572 100644
>>> --- a/arch/x86/kernel/signal.c
>>> +++ b/arch/x86/kernel/signal.c
>>> @@ -102,7 +102,7 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
>>> unsigned long math_size = 0;
>>> unsigned long sp = regs->sp;
>>> unsigned long buf_fx = 0;
>>> - u32 pkru = read_pkru();
>>> + u32 pkru;
>>>
>>> /* redzone */
>>> if (!ia32_frame)
>>> @@ -157,9 +157,17 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
>>> return (void __user *)-1L;
>>> }
>>>
>>> + /* Update PKRU to enable access to the alternate signal stack. */
>>> + pkru = sig_prepare_pkru();
>> I think, the better place to call sig_prepare_pkru() at begging of the
>> get_sigframe:
>>
>> get_sigframe()
>> {
>> /* ... */
>> if (ka->sa.sa_flags & SA_ONSTACK) {
>> if (sas_ss_flags(sp) == 0) {
>> // set pkru = 0 <--- set pkru = 0 here.
>> entering_altstack = true;
>> }
>> }
>> For two reasons:
>> - We probably don't want all signaling handling going through "PKRU=0"
>> , this includes the regular stack and nested signaling handling. The
>> best place is when "entering the altstack". IIUC, this feature only
>> enabled when sigaltstack() is used.
>> - The thread might not have read-access to the altstack, so we will
>> want to make sure that pkru=0 is set before any read to the altstack.
>> And IIRC, fpu__alloc_mathframe needs read-access to the altstack.
>> To help with testing, I will send a test case to demo the usage.
> Apologies, I remembered it wrong, the fpu__alloc_mathframe doesn't
> need read-access to altstack.
>
> But I think the best place to set pkru=0 is still when the stack
> entering altstack, as shown above. the reason is: The signal stack can
> be nested, i.e. trigger another signaling inside signal handler and we
> don't need to set pkru=0 multiple times (only the first time enter
> sigaltstack)
>
>> (please give me sometime to organize the test code, I'm hoping to send
>> out before the end of next week)
>>
> the test code is placed at [1]
> [1] https://github.com/peaktocreek/pkeytest
>
>> Also, could you please consider adding a new flag SS_PKEYALTSTACK (see
>> SS_AUTODISARM for example). Most existing apps that use sigaltstack()
>> don't use PKEY and don't care about setting PKRU=0, and don't need to
>> overwrite the sig frame after XSAVE. The flag will limit the impact
>> of this patch.
>>
> Thanks
> -Jeff
>
Hi Jeff,
I apologize for the delay in my response. I haven’t had a chance to optimize
this patchset or try new test cases.
I agree with your first suggestion that we can set pkru to 0 only when
entering_altstack = true, as that is the intention of this flow anyway.
I’m not so sure if SS_PKEYALTSTACK is really necessary - theoretically it seems
logical to not do this for applications that do not use pkeys but use altstack, but
it adds extra code/checks for possibly insignificant performance gain. I have not
tested this yet.
I’ll try to retest and send out a new patchset on top of the previous ones that have
been merged to 6.12.
Thank you for your feedback, and your attention to detail - I appreciate it.
Thanks,
Aruna
