| Message ID | 20240802061318.2140081-4-aruna.ramakrishna@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/pkeys: update PKRU to enable all pkeys before XSAVE | expand |
On Thu, Aug 1, 2024 at 11:13 PM Aruna Ramakrishna <aruna.ramakrishna@oracle.com> 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@oracle.com> > --- > 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. (please give me sometime to organize the test code, I'm hoping to send out before the end of next week) 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 Best regards, -Jeff > /* save i387 and extended state */ > - if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) > + if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) { > + /* > + * Restore PKRU to the original, user-defined value; disable > + * extra pkeys enabled for the alternate signal stack, if any. > + */ > + write_pkru(pkru); > return (void __user *)-1L; > + } > > return (void __user *)sp; > } > -- > 2.39.3 >
Hi Aruna On Fri, Aug 9, 2024 at 10:30 AM Jeff Xu <jeffxu@chromium.org> wrote: > > On Thu, Aug 1, 2024 at 11:13 PM Aruna Ramakrishna > <aruna.ramakrishna@oracle.com> 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@oracle.com> > > --- > > 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 > Thanks > Best regards, > -Jeff > > > > > > > /* save i387 and extended state */ > > - if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) > > + if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) { > > + /* > > + * Restore PKRU to the original, user-defined value; disable > > + * extra pkeys enabled for the alternate signal stack, if any. > > + */ > > + write_pkru(pkru); > > return (void __user *)-1L; > > + } > > > > return (void __user *)sp; > > } > > -- > > 2.39.3 > >
> On Aug 26, 2024, at 5:53 PM, Jeff Xu <jeffxu@chromium.org> wrote: > > Hi Aruna > > On Fri, Aug 9, 2024 at 10:30 AM Jeff Xu <jeffxu@chromium.org> wrote: >> >> On Thu, Aug 1, 2024 at 11:13 PM Aruna Ramakrishna >> <aruna.ramakrishna@oracle.com> 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@oracle.com> >>> --- >>> 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
Hi Aruna On Thu, Oct 3, 2024 at 4:29 PM Aruna Ramakrishna <aruna.ramakrishna@oracle.com> wrote: > > > > > On Aug 26, 2024, at 5:53 PM, Jeff Xu <jeffxu@chromium.org> wrote: > > > > Hi Aruna > > > > On Fri, Aug 9, 2024 at 10:30 AM Jeff Xu <jeffxu@chromium.org> wrote: > >> > >> On Thu, Aug 1, 2024 at 11:13 PM Aruna Ramakrishna > >> <aruna.ramakrishna@oracle.com> 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@oracle.com> > >>> --- > >>> 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. > Not a problem, I understand. > 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. > One of the reasons that I'm thinking about is that signaling handling has real-time requirements for some real-time systems, and applying PKRU=0 without checking will increase cost to those systems and might be viewed as a regression. Thanks -Jeff > 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 > >
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(); /* save i387 and extended state */ - if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) + if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) { + /* + * Restore PKRU to the original, user-defined value; disable + * extra pkeys enabled for the alternate signal stack, if any. + */ + write_pkru(pkru); return (void __user *)-1L; + } return (void __user *)sp; }
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@oracle.com> --- arch/x86/kernel/fpu/signal.c | 11 +++++++++-- arch/x86/kernel/signal.c | 12 ++++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-)