| Message ID | 20210521221531.30168-7-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Indirect Branch Tracking | expand |
On Fri, 2021-05-21 at 15:15 -0700, Yu-cheng Yu wrote: > From: "H.J. Lu" <hjl.tools@gmail.com> > > Update ARCH_X86_CET_STATUS and ARCH_X86_CET_DISABLE for Indirect > Branch > Tracking. > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > --- > arch/x86/kernel/cet_prctl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/x86/kernel/cet_prctl.c > b/arch/x86/kernel/cet_prctl.c > index b426d200e070..bd3c80d402e7 100644 > --- a/arch/x86/kernel/cet_prctl.c > +++ b/arch/x86/kernel/cet_prctl.c > @@ -22,6 +22,9 @@ static int cet_copy_status_to_user(struct > thread_shstk *shstk, u64 __user *ubuf) > buf[2] = shstk->size; > } > > + if (shstk->ibt) > + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_IBT; > + Can you have IBT enabled but not shadow stack via kernel parameters? Outside this diff it has: if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) return -ENOTSUPP; So if "no_user_shstk" is set, this can't be used for IBT. But the kernel would attempt to enable IBT. Also if so, the CR4 bit enabling logic needs adjusting in this IBT series. If not, we should probably mention this in the docs and enforce it. It would then follow the logic in Kconfig, so maybe the simplest. Like maybe instead of no_user_shstk, just no_user_cet? > return copy_to_user(ubuf, buf, sizeof(buf)); > } > > @@ -46,6 +49,8 @@ int prctl_cet(int option, u64 arg2) > return -EINVAL; > if (arg2 & GNU_PROPERTY_X86_FEATURE_1_SHSTK) > shstk_disable(); > + if (arg2 & GNU_PROPERTY_X86_FEATURE_1_IBT) > + ibt_disable(); > return 0; > > case ARCH_X86_CET_LOCK:
On 7/19/2021 11:21 AM, Edgecombe, Rick P wrote: > On Fri, 2021-05-21 at 15:15 -0700, Yu-cheng Yu wrote: >> From: "H.J. Lu" <hjl.tools@gmail.com> >> >> Update ARCH_X86_CET_STATUS and ARCH_X86_CET_DISABLE for Indirect >> Branch >> Tracking. >> >> Signed-off-by: H.J. Lu <hjl.tools@gmail.com> >> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> >> Reviewed-by: Kees Cook <keescook@chromium.org> >> --- >> arch/x86/kernel/cet_prctl.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/arch/x86/kernel/cet_prctl.c >> b/arch/x86/kernel/cet_prctl.c >> index b426d200e070..bd3c80d402e7 100644 >> --- a/arch/x86/kernel/cet_prctl.c >> +++ b/arch/x86/kernel/cet_prctl.c >> @@ -22,6 +22,9 @@ static int cet_copy_status_to_user(struct >> thread_shstk *shstk, u64 __user *ubuf) >> buf[2] = shstk->size; >> } >> >> + if (shstk->ibt) >> + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_IBT; >> + > Can you have IBT enabled but not shadow stack via kernel parameters? > Outside this diff it has: > if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) > return -ENOTSUPP; If shadow stack is disabled by the kernel parameter, IBT is also disabled. > So if "no_user_shstk" is set, this can't be used for IBT. But the > kernel would attempt to enable IBT. It will not. > Also if so, the CR4 bit enabling logic needs adjusting in this IBT > series. If not, we should probably mention this in the docs and enforce > it. It would then follow the logic in Kconfig, so maybe the simplest. > Like maybe instead of no_user_shstk, just no_user_cet? If shadow stack is disabled (from either Kconfig or kernel command-line), then IBT is also disabled. However, we still need two kernel parameters because no_user_ibt can be useful sometimes. I will add a sentence in the document to indicate that IBT depends on shadow stack. Thanks, Yu-cheng
On Tue, 2021-07-20 at 10:09 -0700, Yu, Yu-cheng wrote: > On 7/19/2021 11:21 AM, Edgecombe, Rick P wrote: > > On Fri, 2021-05-21 at 15:15 -0700, Yu-cheng Yu wrote: > > > From: "H.J. Lu" <hjl.tools@gmail.com> > > > > > > Update ARCH_X86_CET_STATUS and ARCH_X86_CET_DISABLE for Indirect > > > Branch > > > Tracking. > > > > > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > --- > > > arch/x86/kernel/cet_prctl.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/arch/x86/kernel/cet_prctl.c > > > b/arch/x86/kernel/cet_prctl.c > > > index b426d200e070..bd3c80d402e7 100644 > > > --- a/arch/x86/kernel/cet_prctl.c > > > +++ b/arch/x86/kernel/cet_prctl.c > > > @@ -22,6 +22,9 @@ static int cet_copy_status_to_user(struct > > > thread_shstk *shstk, u64 __user *ubuf) > > > buf[2] = shstk->size; > > > } > > > > > > + if (shstk->ibt) > > > + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_IBT; > > > + > > Can you have IBT enabled but not shadow stack via kernel > > parameters? > > Outside this diff it has: > > if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) > > return -ENOTSUPP; > > If shadow stack is disabled by the kernel parameter, IBT is also > disabled. Thanks for the clarification. > > > So if "no_user_shstk" is set, this can't be used for IBT. But the > > kernel would attempt to enable IBT. > > It will not. Oh yea, I see the cpuid_deps part now. Sorry. > > > Also if so, the CR4 bit enabling logic needs adjusting in this IBT > > series. If not, we should probably mention this in the docs and > > enforce > > it. It would then follow the logic in Kconfig, so maybe the > > simplest. > > Like maybe instead of no_user_shstk, just no_user_cet? > > If shadow stack is disabled (from either Kconfig or kernel > command-line), then IBT is also disabled. However, we still need two > kernel parameters because no_user_ibt can be useful sometimes. I > will > add a sentence in the document to indicate that IBT depends on shadow > stack. > > Yea, no_user_ibt seems useful. I meant that renaming no_user_shstk to no_user_cet (or similar) would be more clear and self documenting, since it intends to disable all user cet features and not just shadowstack. And leaving no_user_ibt as is. Documentation works as well though. Not major in any case.
diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c index b426d200e070..bd3c80d402e7 100644 --- a/arch/x86/kernel/cet_prctl.c +++ b/arch/x86/kernel/cet_prctl.c @@ -22,6 +22,9 @@ static int cet_copy_status_to_user(struct thread_shstk *shstk, u64 __user *ubuf) buf[2] = shstk->size; } + if (shstk->ibt) + buf[0] |= GNU_PROPERTY_X86_FEATURE_1_IBT; + return copy_to_user(ubuf, buf, sizeof(buf)); } @@ -46,6 +49,8 @@ int prctl_cet(int option, u64 arg2) return -EINVAL; if (arg2 & GNU_PROPERTY_X86_FEATURE_1_SHSTK) shstk_disable(); + if (arg2 & GNU_PROPERTY_X86_FEATURE_1_IBT) + ibt_disable(); return 0; case ARCH_X86_CET_LOCK: