Re: [RFC PATCH] x86/pkeys: update PKRU to enable pkey 0 before XSAVE

[Thomas Gleixner]

On Fri, Mar 15 2024 at 18:43, Aruna Ramakrishna wrote:
>> On Mar 15, 2024, at 10:36 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>>> But I’m not sure there is a “clean” way to do this. If there is, I’m
>>> happy to redo the patch.
>> 
>> If it turns out to be required, desired whatever then the obvious clean
>> solution is to hand the PKRU value down:
>> 
>>         setup_rt_frame()
>>           xxx_setup_rt_frame()
>>             get_sigframe()
>>               copy_fpstate_to_sigframe()
>> 
>> copy_fpstate_to_sigframe() has the user fpstate pointer already so none
>> of the __update_pkru_in_sigframe() monstrosities are required. No?
>> 
>
> Are you suggesting modifying all these functions down the chain from
> handle_signal() to take in an additional parameter?

Yes.

> Wouldn’t that break kABI?

If so who cares?

kABI is an out of tree madness maintained by distros, but upstream has
never supported it and never will. Aside of that kABI is a driver
interface which hardly has anything to do with the low level
architecture specific signal delivery code.

The kernel has no stable ABI, never had and never will have one, unless
hell freezes over.

> In this approach too, the snippet where the value is modified on the
> sigframe after xsave will remain unchanged, because we need the value
> before xsave to match the register contents.
>
> I guess what I’m saying is, half of __update_pkru_in_sigframe() will
> remain unchanged - it would just be invoked from
> copy_fpstate_to_sigframe() instead of handle_signal().

Yes, but that's the necessary and sane part of it.

> If there’s a way to do this without overwriting PKRU on the sigframe
> after xsave, I'd like to understand that flow.

There is none for obvious reasons unless you figure out how to resolve a
double circular hen and egg problem.

> Or if it’s just a matter of not needing to extract fpstate pointer in
> handle_signal(), I can restructure it that way too.

It's not only the pointer retrieval. Updating xstate is obviously a FPU
specific problem and the general signal handling code simply should not
care. C does not provide encapsulation, but it does not prevent
respecting it either.

Ideally we'd hide all of this in the FPU code, which is anyway the first
one writing to the signal stack. The problem is the error case when any
of the writes after saving the FPU frame terminaly faults or any other
condition makes the signal delivery fail.

So handle_signal() should look like this:

	/* Ensure that the signal stack is writeable */
        pkru = sig_prepare_pkru();

	failed = setup_rt_frame(ksig, regs, pkru);
	if (!failed) {
		/*
		 * Clear the direction flag as per the ABI for function entry.
		 *
		 * Clear RF when entering the signal handler, because
		 * it might disable possible debug exception from the
		 * signal handler.
		 *
		 * Clear TF for the case when it wasn't set by debugger to
		 * avoid the recursive send_sigtrap() in SIGTRAP handler.
		 */
		regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
		/*
		 * Ensure the signal handler starts with the new fpu state.
		 * This also ensures that the PKRU state is set to the
		 * initial state.
		 */
		fpu__clear_user_states(fpu);
	} else {
        	/* Restore the previous PKRU state */
                sig_restore_pkru(pkru);
        }

and then you'd have:

static inline bool copy_fpregs_to_sigframe(struct xregs_state __user *buf, u32 pkru)
{
	if (use_xsave()) {
		if (!xsave_to_user_sigframe(buf))
                	return false;
                if (cpu_feature_enabled(X86_FEATURE_OSPKE))
                	return !__put_user(pkru_address(buf), pkru);
                return true;
        }
	if (use_fxsr())
		return fxsave_to_user_sigframe((struct fxregs_state __user *) buf);
	else
		return fnsave_to_user_sigframe((struct fregs_state __user *) buf);
}

And yes, I deliberately changed the return value of setup_rt_frame() to
bool in this mockup because nothing cares about it. The only relevant
information is whether if failed or not. That want's to be a separate
patch obivously.

Thanks,

        tglx