| Message ID | 20200825002645.3658-7-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Indirect Branch Tracking, PTRACE | expand |
On Tue, Aug 25, 2020 at 2:30 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: > > IA32_U_CET (user-mode CET settings) and > IA32_PL3_SSP (user-mode Shadow Stack) [...] > diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c [...] > +int cetregs_get(struct task_struct *target, const struct user_regset *regset, > + struct membuf to) > +{ > + struct fpu *fpu = &target->thread.fpu; > + struct cet_user_state *cetregs; > + > + if (!boot_cpu_has(X86_FEATURE_SHSTK)) > + return -ENODEV; > + > + fpu__prepare_read(fpu); > + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); > + if (!cetregs) > + return -EFAULT; Can this branch ever be hit without a kernel bug? If yes, I think -EFAULT is probably a weird error code to choose here. If no, this should probably use WARN_ON(). Same thing in cetregs_set(). > + return membuf_write(&to, cetregs, sizeof(struct cet_user_state)); > +} [...] > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c [...] > @@ -52,7 +52,9 @@ enum x86_regset { > REGSET_IOPERM64 = REGSET_XFP, > REGSET_XSTATE, > REGSET_TLS, > + REGSET_CET64 = REGSET_TLS, > REGSET_IOPERM32, > + REGSET_CET32, > }; [...] > @@ -1229,6 +1231,13 @@ static struct user_regset x86_64_regsets[] __ro_after_init = { [...] > + [REGSET_CET64] = { > + .core_note_type = NT_X86_CET, > + .n = sizeof(struct cet_user_state) / sizeof(u64), > + .size = sizeof(u64), .align = sizeof(u64), > + .active = cetregs_active, .regset_get = cetregs_get, > + .set = cetregs_set > + }, > }; [...] > @@ -1284,6 +1293,13 @@ static struct user_regset x86_32_regsets[] __ro_after_init = { [...] > + [REGSET_CET32] = { > + .core_note_type = NT_X86_CET, > + .n = sizeof(struct cet_user_state) / sizeof(u64), > + .size = sizeof(u64), .align = sizeof(u64), > + .active = cetregs_active, .regset_get = cetregs_get, > + .set = cetregs_set > + }, > }; Why are there different identifiers for 32-bit CET and 64-bit CET when they operate on the same structs and have the same handlers? If there's a good reason for that, the commit message should probably point that out.
On 9/2/2020 1:03 PM, Jann Horn wrote: > On Tue, Aug 25, 2020 at 2:30 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >> Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: >> >> IA32_U_CET (user-mode CET settings) and >> IA32_PL3_SSP (user-mode Shadow Stack) > [...] >> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c > [...] >> +int cetregs_get(struct task_struct *target, const struct user_regset *regset, >> + struct membuf to) >> +{ >> + struct fpu *fpu = &target->thread.fpu; >> + struct cet_user_state *cetregs; >> + >> + if (!boot_cpu_has(X86_FEATURE_SHSTK)) >> + return -ENODEV; >> + >> + fpu__prepare_read(fpu); >> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >> + if (!cetregs) >> + return -EFAULT; > > Can this branch ever be hit without a kernel bug? If yes, I think > -EFAULT is probably a weird error code to choose here. If no, this > should probably use WARN_ON(). Same thing in cetregs_set(). > When a thread is not CET-enabled, its CET state does not exist. I looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, which means "No such device"? [...] >> @@ -1284,6 +1293,13 @@ static struct user_regset x86_32_regsets[] __ro_after_init = { > [...] >> + [REGSET_CET32] = { >> + .core_note_type = NT_X86_CET, >> + .n = sizeof(struct cet_user_state) / sizeof(u64), >> + .size = sizeof(u64), .align = sizeof(u64), >> + .active = cetregs_active, .regset_get = cetregs_get, >> + .set = cetregs_set >> + }, >> }; > > Why are there different identifiers for 32-bit CET and 64-bit CET when > they operate on the same structs and have the same handlers? If > there's a good reason for that, the commit message should probably > point that out. > Yes, the reason for two regsets is that fill_note_info() does not expect any holes in a regsets. I will put this in the commit log. Thanks, Yu-cheng
> On Sep 2, 2020, at 3:13 PM, Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > > On 9/2/2020 1:03 PM, Jann Horn wrote: >>> On Tue, Aug 25, 2020 at 2:30 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>> Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: >>> >>> IA32_U_CET (user-mode CET settings) and >>> IA32_PL3_SSP (user-mode Shadow Stack) >> [...] >>> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c >> [...] >>> +int cetregs_get(struct task_struct *target, const struct user_regset *regset, >>> + struct membuf to) >>> +{ >>> + struct fpu *fpu = &target->thread.fpu; >>> + struct cet_user_state *cetregs; >>> + >>> + if (!boot_cpu_has(X86_FEATURE_SHSTK)) >>> + return -ENODEV; >>> + >>> + fpu__prepare_read(fpu); >>> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >>> + if (!cetregs) >>> + return -EFAULT; >> Can this branch ever be hit without a kernel bug? If yes, I think >> -EFAULT is probably a weird error code to choose here. If no, this >> should probably use WARN_ON(). Same thing in cetregs_set(). > > When a thread is not CET-enabled, its CET state does not exist. I looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, which means "No such device"? > > [...] > >>> @@ -1284,6 +1293,13 @@ static struct user_regset x86_32_regsets[] __ro_after_init = { >> [...] >>> + [REGSET_CET32] = { >>> + .core_note_type = NT_X86_CET, >>> + .n = sizeof(struct cet_user_state) / sizeof(u64), >>> + .size = sizeof(u64), .align = sizeof(u64), >>> + .active = cetregs_active, .regset_get = cetregs_get, >>> + .set = cetregs_set >>> + }, >>> }; >> Why are there different identifiers for 32-bit CET and 64-bit CET when >> they operate on the same structs and have the same handlers? If >> there's a good reason for that, the commit message should probably >> point that out. > > Yes, the reason for two regsets is that fill_note_info() does not expect any holes in a regsets. I will put this in the commit log. > > Perhaps we could fix that instead?
On Thu, Sep 3, 2020 at 12:13 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > On 9/2/2020 1:03 PM, Jann Horn wrote: > > On Tue, Aug 25, 2020 at 2:30 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > >> Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: > >> > >> IA32_U_CET (user-mode CET settings) and > >> IA32_PL3_SSP (user-mode Shadow Stack) > > [...] > >> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c > > [...] > >> +int cetregs_get(struct task_struct *target, const struct user_regset *regset, > >> + struct membuf to) > >> +{ > >> + struct fpu *fpu = &target->thread.fpu; > >> + struct cet_user_state *cetregs; > >> + > >> + if (!boot_cpu_has(X86_FEATURE_SHSTK)) > >> + return -ENODEV; > >> + > >> + fpu__prepare_read(fpu); > >> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); > >> + if (!cetregs) > >> + return -EFAULT; > > > > Can this branch ever be hit without a kernel bug? If yes, I think > > -EFAULT is probably a weird error code to choose here. If no, this > > should probably use WARN_ON(). Same thing in cetregs_set(). > > > > When a thread is not CET-enabled, its CET state does not exist. I > looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, > which means "No such device"? Yeah, I guess ENODEV might fit reasonably well.
On 9/2/2020 4:50 PM, Andy Lutomirski wrote: > > >> On Sep 2, 2020, at 3:13 PM, Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >> >> On 9/2/2020 1:03 PM, Jann Horn wrote: >>>> On Tue, Aug 25, 2020 at 2:30 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>>> Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: >>>> >>>> IA32_U_CET (user-mode CET settings) and >>>> IA32_PL3_SSP (user-mode Shadow Stack) >>> [...] >>>> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c >>> [...] >>>> +int cetregs_get(struct task_struct *target, const struct user_regset *regset, >>>> + struct membuf to) >>>> +{ >>>> + struct fpu *fpu = &target->thread.fpu; >>>> + struct cet_user_state *cetregs; >>>> + >>>> + if (!boot_cpu_has(X86_FEATURE_SHSTK)) >>>> + return -ENODEV; >>>> + >>>> + fpu__prepare_read(fpu); >>>> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >>>> + if (!cetregs) >>>> + return -EFAULT; >>> Can this branch ever be hit without a kernel bug? If yes, I think >>> -EFAULT is probably a weird error code to choose here. If no, this >>> should probably use WARN_ON(). Same thing in cetregs_set(). >> >> When a thread is not CET-enabled, its CET state does not exist. I looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, which means "No such device"? >> >> [...] >> >>>> @@ -1284,6 +1293,13 @@ static struct user_regset x86_32_regsets[] __ro_after_init = { >>> [...] >>>> + [REGSET_CET32] = { >>>> + .core_note_type = NT_X86_CET, >>>> + .n = sizeof(struct cet_user_state) / sizeof(u64), >>>> + .size = sizeof(u64), .align = sizeof(u64), >>>> + .active = cetregs_active, .regset_get = cetregs_get, >>>> + .set = cetregs_set >>>> + }, >>>> }; >>> Why are there different identifiers for 32-bit CET and 64-bit CET when >>> they operate on the same structs and have the same handlers? If >>> there's a good reason for that, the commit message should probably >>> point that out. >> >> Yes, the reason for two regsets is that fill_note_info() does not expect any holes in a regsets. I will put this in the commit log. >> >> > > Perhaps we could fix that instead? > As long as we understand the root cause, leaving it as-is may be OK. I had a patch in the past, but did not follow up on it. https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/ Yu-cheng
On 9/2/2020 5:33 PM, Jann Horn wrote: > On Thu, Sep 3, 2020 at 12:13 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >> On 9/2/2020 1:03 PM, Jann Horn wrote: >>> On Tue, Aug 25, 2020 at 2:30 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>>> Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: >>>> >>>> IA32_U_CET (user-mode CET settings) and >>>> IA32_PL3_SSP (user-mode Shadow Stack) >>> [...] >>>> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c >>> [...] >>>> +int cetregs_get(struct task_struct *target, const struct user_regset *regset, >>>> + struct membuf to) >>>> +{ >>>> + struct fpu *fpu = &target->thread.fpu; >>>> + struct cet_user_state *cetregs; >>>> + >>>> + if (!boot_cpu_has(X86_FEATURE_SHSTK)) >>>> + return -ENODEV; >>>> + >>>> + fpu__prepare_read(fpu); >>>> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >>>> + if (!cetregs) >>>> + return -EFAULT; >>> >>> Can this branch ever be hit without a kernel bug? If yes, I think >>> -EFAULT is probably a weird error code to choose here. If no, this >>> should probably use WARN_ON(). Same thing in cetregs_set(). >>> >> >> When a thread is not CET-enabled, its CET state does not exist. I >> looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, >> which means "No such device"? > > Yeah, I guess ENODEV might fit reasonably well. > I will update it. Thanks!
> On Sep 2, 2020, at 7:53 PM, Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > > On 9/2/2020 4:50 PM, Andy Lutomirski wrote: >>>> On Sep 2, 2020, at 3:13 PM, Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: >>> >>> On 9/2/2020 1:03 PM, Jann Horn wrote: >>>>> On Tue, Aug 25, 2020 at 2:30 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >>>>> Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: >>>>> >>>>> IA32_U_CET (user-mode CET settings) and >>>>> IA32_PL3_SSP (user-mode Shadow Stack) >>>> [...] >>>>> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c >>>> [...] >>>>> +int cetregs_get(struct task_struct *target, const struct user_regset *regset, >>>>> + struct membuf to) >>>>> +{ >>>>> + struct fpu *fpu = &target->thread.fpu; >>>>> + struct cet_user_state *cetregs; >>>>> + >>>>> + if (!boot_cpu_has(X86_FEATURE_SHSTK)) >>>>> + return -ENODEV; >>>>> + >>>>> + fpu__prepare_read(fpu); >>>>> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >>>>> + if (!cetregs) >>>>> + return -EFAULT; >>>> Can this branch ever be hit without a kernel bug? If yes, I think >>>> -EFAULT is probably a weird error code to choose here. If no, this >>>> should probably use WARN_ON(). Same thing in cetregs_set(). >>> >>> When a thread is not CET-enabled, its CET state does not exist. I looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, which means "No such device"? Having read the code, I’m unconvinced. It looks like a get_xsave_addr() failure means “state not saved; task sees INIT state”. So *maybe* it’s reasonable -ENODEV this, but I’m not really convinced. I tend to think we should return the actual INIT state and that we should permit writes and handle them correctly. Dave, what do you think? >>> >>> [...] >>> >>>>> @@ -1284,6 +1293,13 @@ static struct user_regset x86_32_regsets[] __ro_after_init = { >>>> [...] >>>>> + [REGSET_CET32] = { >>>>> + .core_note_type = NT_X86_CET, >>>>> + .n = sizeof(struct cet_user_state) / sizeof(u64), >>>>> + .size = sizeof(u64), .align = sizeof(u64), >>>>> + .active = cetregs_active, .regset_get = cetregs_get, >>>>> + .set = cetregs_set >>>>> + }, >>>>> }; >>>> Why are there different identifiers for 32-bit CET and 64-bit CET when >>>> they operate on the same structs and have the same handlers? If >>>> there's a good reason for that, the commit message should probably >>>> point that out. >>> >>> Yes, the reason for two regsets is that fill_note_info() does not expect any holes in a regsets. I will put this in the commit log. >>> >>> >> Perhaps we could fix that instead? > > As long as we understand the root cause, leaving it as-is may be OK. The regset mechanism’s interactions with compat are awful. Let’s please not make it worse. One CET regret is good; two is not good. > > I had a patch in the past, but did not follow up on it. > > https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/ > > Yu-cheng
On 9/2/20 9:35 PM, Andy Lutomirski wrote: >>>>>> + fpu__prepare_read(fpu); >>>>>> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >>>>>> + if (!cetregs) >>>>>> + return -EFAULT; >>>>> Can this branch ever be hit without a kernel bug? If yes, I think >>>>> -EFAULT is probably a weird error code to choose here. If no, this >>>>> should probably use WARN_ON(). Same thing in cetregs_set(). >>>> When a thread is not CET-enabled, its CET state does not exist. I looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, which means "No such device"? > Having read the code, I’m unconvinced. It looks like a get_xsave_addr() failure means “state not saved; task sees INIT state”. So *maybe* it’s reasonable -ENODEV this, but I’m not really convinced. I tend to think we should return the actual INIT state and that we should permit writes and handle them correctly. PTRACE is asking for access to the values in the *registers*, not for the value in the kernel XSAVE buffer. We just happen to only have the kernel XSAVE buffer around. If we want to really support PTRACE we have to allow the registers to be get/set, regardless of what state they are in, INIT state or not. So, yeah I agree with Andy.
On Thu, Sep 3, 2020 at 7:27 AM Dave Hansen <dave.hansen@intel.com> wrote: > > On 9/2/20 9:35 PM, Andy Lutomirski wrote: > >>>>>> + fpu__prepare_read(fpu); > >>>>>> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); > >>>>>> + if (!cetregs) > >>>>>> + return -EFAULT; > >>>>> Can this branch ever be hit without a kernel bug? If yes, I think > >>>>> -EFAULT is probably a weird error code to choose here. If no, this > >>>>> should probably use WARN_ON(). Same thing in cetregs_set(). > >>>> When a thread is not CET-enabled, its CET state does not exist. I looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, which means "No such device"? > > Having read the code, I’m unconvinced. It looks like a get_xsave_addr() failure means “state not saved; task sees INIT state”. So *maybe* it’s reasonable -ENODEV this, but I’m not really convinced. I tend to think we should return the actual INIT state and that we should permit writes and handle them correctly. > > PTRACE is asking for access to the values in the *registers*, not for > the value in the kernel XSAVE buffer. We just happen to only have the > kernel XSAVE buffer around. > > If we want to really support PTRACE we have to allow the registers to be > get/set, regardless of what state they are in, INIT state or not. So, > yeah I agree with Andy. I think the core dump code gets here, too, so the values might be in registers as well. I hope that fpu__prepare_read() does the right thing in this case. --Andy
On 9/3/2020 7:26 AM, Dave Hansen wrote: > On 9/2/20 9:35 PM, Andy Lutomirski wrote: >>>>>>> + fpu__prepare_read(fpu); >>>>>>> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >>>>>>> + if (!cetregs) >>>>>>> + return -EFAULT; >>>>>> Can this branch ever be hit without a kernel bug? If yes, I think >>>>>> -EFAULT is probably a weird error code to choose here. If no, this >>>>>> should probably use WARN_ON(). Same thing in cetregs_set(). >>>>> When a thread is not CET-enabled, its CET state does not exist. I looked at EFAULT, and it means "Bad address". Maybe this can be ENODEV, which means "No such device"? >> Having read the code, I’m unconvinced. It looks like a get_xsave_addr() failure means “state not saved; task sees INIT state”. So *maybe* it’s reasonable -ENODEV this, but I’m not really convinced. I tend to think we should return the actual INIT state and that we should permit writes and handle them correctly. > > PTRACE is asking for access to the values in the *registers*, not for > the value in the kernel XSAVE buffer. We just happen to only have the > kernel XSAVE buffer around. When get_xsave_addr() returns NULL, there are three possibilities: - XSAVE is not enabled or not supported; - The kernel does not support the requested feature; - The requested feature is in INIT state. If the debugger is going to write an MSR, only in the third case would this make a slight sense. For example, if the system has CET enabled, but the task does not have CET enabled, and GDB is writing to a CET MSR. But still, this is strange to me. > > If we want to really support PTRACE we have to allow the registers to be > get/set, regardless of what state they are in, INIT state or not. So, > yeah I agree with Andy. > GDB does not have a WRMSR mechanism. If GDB is going to write an MSR, it will call arch_prctl or an assembly routine in memory. Yu-cheng
On 9/3/20 9:09 AM, Yu, Yu-cheng wrote: > If the debugger is going to write an MSR, only in the third case would > this make a slight sense. For example, if the system has CET enabled, > but the task does not have CET enabled, and GDB is writing to a CET MSR. > But still, this is strange to me. If this is strange, then why do we even _implement_ writes?
On Thu, Sep 3, 2020 at 9:12 AM Dave Hansen <dave.hansen@intel.com> wrote: > > On 9/3/20 9:09 AM, Yu, Yu-cheng wrote: > > If the debugger is going to write an MSR, only in the third case would > > this make a slight sense. For example, if the system has CET enabled, > > but the task does not have CET enabled, and GDB is writing to a CET MSR. > > But still, this is strange to me. > > If this is strange, then why do we even _implement_ writes? Well, if gdb wants to force a tracee to return early from a function, wouldn't it want the ability to modify SSP? --Andy
On 9/3/2020 9:11 AM, Dave Hansen wrote: > On 9/3/20 9:09 AM, Yu, Yu-cheng wrote: >> If the debugger is going to write an MSR, only in the third case would >> this make a slight sense. For example, if the system has CET enabled, >> but the task does not have CET enabled, and GDB is writing to a CET MSR. >> But still, this is strange to me. > > If this is strange, then why do we even _implement_ writes? > For example, if the task has CET enabled, and it is in a control protection fault, the debugger can clear the task's IBT state, or unwind the shadow stack, etc. But if the task does not have CET enabled (its CET MSRs are in INIT state), it would make sense for the PTRACE call to return failure than doing something else, right?
On Thu, Sep 3, 2020 at 9:21 AM Yu, Yu-cheng <yu-cheng.yu@intel.com> wrote: > > On 9/3/2020 9:11 AM, Dave Hansen wrote: > > On 9/3/20 9:09 AM, Yu, Yu-cheng wrote: > >> If the debugger is going to write an MSR, only in the third case would > >> this make a slight sense. For example, if the system has CET enabled, > >> but the task does not have CET enabled, and GDB is writing to a CET MSR. > >> But still, this is strange to me. > > > > If this is strange, then why do we even _implement_ writes? > > > > For example, if the task has CET enabled, and it is in a control > protection fault, the debugger can clear the task's IBT state, or unwind > the shadow stack, etc. But if the task does not have CET enabled (its > CET MSRs are in INIT state), it would make sense for the PTRACE call to > return failure than doing something else, right? What do you mean "something else"? I assume that, if GDB tells ptrace() to write some value to the CET MSR, then it should get that value. If GDB writes to it on a task that is not currently using CET, I don't see why it should fail. --Andy
On 9/3/20 9:15 AM, Andy Lutomirski wrote: > On Thu, Sep 3, 2020 at 9:12 AM Dave Hansen <dave.hansen@intel.com> wrote: >> >> On 9/3/20 9:09 AM, Yu, Yu-cheng wrote: >>> If the debugger is going to write an MSR, only in the third case would >>> this make a slight sense. For example, if the system has CET enabled, >>> but the task does not have CET enabled, and GDB is writing to a CET MSR. >>> But still, this is strange to me. >> >> If this is strange, then why do we even _implement_ writes? > > Well, if gdb wants to force a tracee to return early from a function, > wouldn't it want the ability to modify SSP? That's true. Yu-cheng, can you take a look through and see for the other setregs users what they do for logically crazy, strange things? Is there precedent for refusing a write which is possible but illogical or strange? If so, which error code do they use? Taking the config register out of the init state is illogical, as is writing to SSP while the config register is in its init state.
On Thu, Sep 3, 2020 at 9:25 AM Dave Hansen <dave.hansen@intel.com> wrote: > > On 9/3/20 9:15 AM, Andy Lutomirski wrote: > > On Thu, Sep 3, 2020 at 9:12 AM Dave Hansen <dave.hansen@intel.com> wrote: > >> > >> On 9/3/20 9:09 AM, Yu, Yu-cheng wrote: > >>> If the debugger is going to write an MSR, only in the third case would > >>> this make a slight sense. For example, if the system has CET enabled, > >>> but the task does not have CET enabled, and GDB is writing to a CET MSR. > >>> But still, this is strange to me. > >> > >> If this is strange, then why do we even _implement_ writes? > > > > Well, if gdb wants to force a tracee to return early from a function, > > wouldn't it want the ability to modify SSP? > > That's true. > > Yu-cheng, can you take a look through and see for the other setregs > users what they do for logically crazy, strange things? Is there > precedent for refusing a write which is possible but illogical or > strange? If so, which error code do they use? > > Taking the config register out of the init state is illogical, as is > writing to SSP while the config register is in its init state. What's so special about the INIT state? It's optimized by XSAVES, but it's just a number, right? So taking the register out of the INIT state is kind of like saying "gdb wanted to set xmm0 to (0,0,0,1), but it was in the INIT state to begin with", right?
On 9/3/20 9:32 AM, Andy Lutomirski wrote: >> Taking the config register out of the init state is illogical, as is >> writing to SSP while the config register is in its init state. > What's so special about the INIT state? It's optimized by XSAVES, but > it's just a number, right? So taking the register out of the INIT > state is kind of like saying "gdb wanted to set xmm0 to (0,0,0,1), but > it was in the INIT state to begin with", right? Yeah, that's a good point. The init state shouldn't be special, as the hardware is within its right to choose not to use the init optimization at any time.
On 9/3/2020 9:42 AM, Dave Hansen wrote: > On 9/3/20 9:32 AM, Andy Lutomirski wrote: >>> Taking the config register out of the init state is illogical, as is >>> writing to SSP while the config register is in its init state. >> What's so special about the INIT state? It's optimized by XSAVES, but >> it's just a number, right? So taking the register out of the INIT >> state is kind of like saying "gdb wanted to set xmm0 to (0,0,0,1), but >> it was in the INIT state to begin with", right? > > Yeah, that's a good point. The init state shouldn't be special, as the > hardware is within its right to choose not to use the init optimization > at any time. > Then, I would suggest changing get_xsave_addr() to return non-null for the INIT state case. For the other two cases, it still returns NULL. But this also requires any write to INIT states to set xstate_bv bits properly. This would be a pitfall for any code addition later on. Looking at this another way. Would it be better for the debugger to get an error and then to set the MSR directly first (vs. changing the XSAVES INIT state first)?
diff --git a/arch/x86/include/asm/fpu/regset.h b/arch/x86/include/asm/fpu/regset.h index 4f928d6a367b..8622184d87f5 100644 --- a/arch/x86/include/asm/fpu/regset.h +++ b/arch/x86/include/asm/fpu/regset.h @@ -7,11 +7,12 @@ #include <linux/regset.h> -extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active; +extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active, + cetregs_active; extern user_regset_get2_fn fpregs_get, xfpregs_get, fpregs_soft_get, - xstateregs_get; + xstateregs_get, cetregs_get; extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set, - xstateregs_set; + xstateregs_set, cetregs_set; /* * xstateregs_active == regset_fpregs_active. Please refer to the comment diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c index c413756ba89f..8860d57eed35 100644 --- a/arch/x86/kernel/fpu/regset.c +++ b/arch/x86/kernel/fpu/regset.c @@ -149,6 +149,50 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset, return ret; } +int cetregs_active(struct task_struct *target, const struct user_regset *regset) +{ +#ifdef CONFIG_X86_INTEL_CET + if (target->thread.cet.shstk_size || target->thread.cet.ibt_enabled) + return regset->n; +#endif + return 0; +} + +int cetregs_get(struct task_struct *target, const struct user_regset *regset, + struct membuf to) +{ + struct fpu *fpu = &target->thread.fpu; + struct cet_user_state *cetregs; + + if (!boot_cpu_has(X86_FEATURE_SHSTK)) + return -ENODEV; + + fpu__prepare_read(fpu); + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); + if (!cetregs) + return -EFAULT; + + return membuf_write(&to, cetregs, sizeof(struct cet_user_state)); +} + +int cetregs_set(struct task_struct *target, const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + struct fpu *fpu = &target->thread.fpu; + struct cet_user_state *cetregs; + + if (!boot_cpu_has(X86_FEATURE_SHSTK)) + return -ENODEV; + + fpu__prepare_write(fpu); + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); + if (!cetregs) + return -EFAULT; + + return user_regset_copyin(&pos, &count, &kbuf, &ubuf, cetregs, 0, -1); +} + #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION /* diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index 5679aa3fdcb8..ea54317f087e 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -52,7 +52,9 @@ enum x86_regset { REGSET_IOPERM64 = REGSET_XFP, REGSET_XSTATE, REGSET_TLS, + REGSET_CET64 = REGSET_TLS, REGSET_IOPERM32, + REGSET_CET32, }; struct pt_regs_offset { @@ -1229,6 +1231,13 @@ static struct user_regset x86_64_regsets[] __ro_after_init = { .size = sizeof(long), .align = sizeof(long), .active = ioperm_active, .regset_get = ioperm_get }, + [REGSET_CET64] = { + .core_note_type = NT_X86_CET, + .n = sizeof(struct cet_user_state) / sizeof(u64), + .size = sizeof(u64), .align = sizeof(u64), + .active = cetregs_active, .regset_get = cetregs_get, + .set = cetregs_set + }, }; static const struct user_regset_view user_x86_64_view = { @@ -1284,6 +1293,13 @@ static struct user_regset x86_32_regsets[] __ro_after_init = { .size = sizeof(u32), .align = sizeof(u32), .active = ioperm_active, .regset_get = ioperm_get }, + [REGSET_CET32] = { + .core_note_type = NT_X86_CET, + .n = sizeof(struct cet_user_state) / sizeof(u64), + .size = sizeof(u64), .align = sizeof(u64), + .active = cetregs_active, .regset_get = cetregs_get, + .set = cetregs_set + }, }; static const struct user_regset_view user_x86_32_view = { diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index ca5875f384f6..d2a895369bcc 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -402,6 +402,7 @@ typedef struct elf64_shdr { #define NT_386_TLS 0x200 /* i386 TLS slots (struct user_desc) */ #define NT_386_IOPERM 0x201 /* x86 io permission bitmap (1=deny) */ #define NT_X86_XSTATE 0x202 /* x86 extended state using xsave */ +#define NT_X86_CET 0x203 /* x86 cet state */ #define NT_S390_HIGH_GPRS 0x300 /* s390 upper register halves */ #define NT_S390_TIMER 0x301 /* s390 timer register */ #define NT_S390_TODCMP 0x302 /* s390 TOD clock comparator register */
Add REGSET_CET64/REGSET_CET32 to get/set CET MSRs: IA32_U_CET (user-mode CET settings) and IA32_PL3_SSP (user-mode Shadow Stack) Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/include/asm/fpu/regset.h | 7 ++--- arch/x86/kernel/fpu/regset.c | 44 +++++++++++++++++++++++++++++++ arch/x86/kernel/ptrace.c | 16 +++++++++++ include/uapi/linux/elf.h | 1 + 4 files changed, 65 insertions(+), 3 deletions(-)