| Message ID | 20180921150553.21016-7-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control Flow Enforcement: Branch Tracking, PTRACE | expand |
On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote: > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect > Branch Tracking features. > > Introduce: > > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr) > Enable the Indirect Branch Tracking legacy code bitmap. > > The parameter 'addr' is a pointer to a user buffer. > On returning to the caller, the kernel fills the following: > > *addr = IBT bitmap base address > *(addr + 1) = IBT bitmap size Again, some structure with a size field would be better from UAPI/extensibility standpoint. One additional point: "size" in the structure from kernel should have structure size expected by kernel, and at least providing there "0" from user space shouldn't lead to failure (in fact, it is possible to provide structure size back to userspace even if buffer is too small, along with error). > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > --- > arch/x86/include/uapi/asm/prctl.h | 1 + > arch/x86/kernel/cet_prctl.c | 38 ++++++++++++++++++++++++++++++- > arch/x86/kernel/process.c | 1 + > 3 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h > index 3aec1088e01d..31d2465f9caf 100644 > --- a/arch/x86/include/uapi/asm/prctl.h > +++ b/arch/x86/include/uapi/asm/prctl.h > @@ -18,5 +18,6 @@ > #define ARCH_CET_DISABLE 0x3002 > #define ARCH_CET_LOCK 0x3003 > #define ARCH_CET_ALLOC_SHSTK 0x3004 > +#define ARCH_CET_LEGACY_BITMAP 0x3005 It would probably be nice to have mention of an architecture in these definitions ("ARCH_X86_CET_"...), but it's likely too late. > > #endif /* _ASM_X86_PRCTL_H */ > diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c > index c4b7c19f5040..df47b5ebc3f4 100644 > --- a/arch/x86/kernel/cet_prctl.c > +++ b/arch/x86/kernel/cet_prctl.c > @@ -20,6 +20,8 @@ static int handle_get_status(unsigned long arg2) > > if (current->thread.cet.shstk_enabled) > features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK; > + if (current->thread.cet.ibt_enabled) > + features |= GNU_PROPERTY_X86_FEATURE_1_IBT; > > shstk_base = current->thread.cet.shstk_base; > shstk_size = current->thread.cet.shstk_size; > @@ -49,9 +51,35 @@ static int handle_alloc_shstk(unsigned long arg2) > return 0; > } > > +static int handle_bitmap(unsigned long arg2) > +{ > + unsigned long addr, size; > + > + if (current->thread.cet.ibt_enabled) { > + int err; > + > + err = cet_setup_ibt_bitmap(); > + if (err) > + return err; > + > + addr = current->thread.cet.ibt_bitmap_addr; > + size = current->thread.cet.ibt_bitmap_size; > + } else { > + addr = 0; > + size = 0; > + } > + > + if (put_user(addr, (unsigned long __user *)arg2) || > + put_user(size, (unsigned long __user *)arg2 + 1)) > + return -EFAULT; > + > + return 0; > +} > + > int prctl_cet(int option, unsigned long arg2) > { > - if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) && > + !cpu_feature_enabled(X86_FEATURE_IBT)) This check is repeated many times, it is probably worth defining something like cpu_x86_cet_enabled() or something like that. Besides, early introduction of the macro would allow avoiding all these changes over the code in IBT patches, only macro definition has to be changed that way. > @@ -73,6 +103,12 @@ int prctl_cet(int option, unsigned long arg2) > case ARCH_CET_ALLOC_SHSTK: > return handle_alloc_shstk(arg2); > > + /* > + * Allocate legacy bitmap and return address & size to user. > + */ > + case ARCH_CET_LEGACY_BITMAP: > + return handle_bitmap(arg2); > + > default: > return -EINVAL; > } > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > index ac0ea9c7e89f..aea15a9b6a3e 100644 > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -797,6 +797,7 @@ long do_arch_prctl_common(struct task_struct *task, int option, > case ARCH_CET_DISABLE: > case ARCH_CET_LOCK: > case ARCH_CET_ALLOC_SHSTK: > + case ARCH_CET_LEGACY_BITMAP: > return prctl_cet(option, cpuid_enabled); > } I wonder, whether this duplication is really needed for CET-related arch_prctl commands, why not just call them from do_arch_prctl_common?
On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote: > On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote: > > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect > > Branch Tracking features. > > > > Introduce: > > > > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr) > > Enable the Indirect Branch Tracking legacy code bitmap. > > > > The parameter 'addr' is a pointer to a user buffer. > > On returning to the caller, the kernel fills the following: > > > > *addr = IBT bitmap base address > > *(addr + 1) = IBT bitmap size > > Again, some structure with a size field would be better from > UAPI/extensibility standpoint. > > One additional point: "size" in the structure from kernel should have > structure size expected by kernel, and at least providing there "0" from > user space shouldn't lead to failure (in fact, it is possible to provide > structure size back to userspace even if buffer is too small, along > with error). This has been in GLIBC v2.28. We cannot change it anymore. > > > > > Signed-off-by: H.J. Lu <hjl.tools@gmail.com> > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > --- > > arch/x86/include/uapi/asm/prctl.h | 1 + > > arch/x86/kernel/cet_prctl.c | 38 ++++++++++++++++++++++++++++++- > > arch/x86/kernel/process.c | 1 + > > 3 files changed, 39 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/uapi/asm/prctl.h > > b/arch/x86/include/uapi/asm/prctl.h > > index 3aec1088e01d..31d2465f9caf 100644 > > --- a/arch/x86/include/uapi/asm/prctl.h > > +++ b/arch/x86/include/uapi/asm/prctl.h > > @@ -18,5 +18,6 @@ > > #define ARCH_CET_DISABLE 0x3002 > > #define ARCH_CET_LOCK 0x3003 > > #define ARCH_CET_ALLOC_SHSTK 0x3004 > > +#define ARCH_CET_LEGACY_BITMAP 0x3005 > > It would probably be nice to have mention of an architecture in these > definitions ("ARCH_X86_CET_"...), but it's likely too late. We can still change macro names. I will work on that. > > > > > #endif /* _ASM_X86_PRCTL_H */ > > diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c > > index c4b7c19f5040..df47b5ebc3f4 100644 > > --- a/arch/x86/kernel/cet_prctl.c > > +++ b/arch/x86/kernel/cet_prctl.c > > @@ -20,6 +20,8 @@ static int handle_get_status(unsigned long arg2) > > > > if (current->thread.cet.shstk_enabled) > > features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK; > > + if (current->thread.cet.ibt_enabled) > > + features |= GNU_PROPERTY_X86_FEATURE_1_IBT; > > > > shstk_base = current->thread.cet.shstk_base; > > shstk_size = current->thread.cet.shstk_size; > > @@ -49,9 +51,35 @@ static int handle_alloc_shstk(unsigned long arg2) > > return 0; > > } > > > > +static int handle_bitmap(unsigned long arg2) > > +{ > > + unsigned long addr, size; > > + > > + if (current->thread.cet.ibt_enabled) { > > + int err; > > + > > + err = cet_setup_ibt_bitmap(); > > + if (err) > > + return err; > > + > > + addr = current->thread.cet.ibt_bitmap_addr; > > + size = current->thread.cet.ibt_bitmap_size; > > + } else { > > + addr = 0; > > + size = 0; > > + } > > + > > + if (put_user(addr, (unsigned long __user *)arg2) || > > + put_user(size, (unsigned long __user *)arg2 + 1)) > > + return -EFAULT; > > + > > + return 0; > > +} > > + > > int prctl_cet(int option, unsigned long arg2) > > { > > - if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) > > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) && > > + !cpu_feature_enabled(X86_FEATURE_IBT)) > > This check is repeated many times, it is probably worth defining > something like cpu_x86_cet_enabled() or something like that. > Besides, early introduction of the macro would allow avoiding all these > changes over the code in IBT patches, only macro definition has > to be changed that way. Yes, that makes things easier. > > > @@ -73,6 +103,12 @@ int prctl_cet(int option, unsigned long arg2) > > case ARCH_CET_ALLOC_SHSTK: > > return handle_alloc_shstk(arg2); > > > > + /* > > + * Allocate legacy bitmap and return address & size to user. > > + */ > > + case ARCH_CET_LEGACY_BITMAP: > > + return handle_bitmap(arg2); > > + > > default: > > return -EINVAL; > > } > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > > index ac0ea9c7e89f..aea15a9b6a3e 100644 > > --- a/arch/x86/kernel/process.c > > +++ b/arch/x86/kernel/process.c > > @@ -797,6 +797,7 @@ long do_arch_prctl_common(struct task_struct *task, int > > option, > > case ARCH_CET_DISABLE: > > case ARCH_CET_LOCK: > > case ARCH_CET_ALLOC_SHSTK: > > + case ARCH_CET_LEGACY_BITMAP: > > return prctl_cet(option, cpuid_enabled); > > } > > I wonder, whether this duplication is really needed for CET-related > arch_prctl commands, why not just call them from do_arch_prctl_common? I will fix it. Yu-cheng
* Yu-cheng Yu: > On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote: >> On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote: >> > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect >> > Branch Tracking features. >> > >> > Introduce: >> > >> > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr) >> > Enable the Indirect Branch Tracking legacy code bitmap. >> > >> > The parameter 'addr' is a pointer to a user buffer. >> > On returning to the caller, the kernel fills the following: >> > >> > *addr = IBT bitmap base address >> > *(addr + 1) = IBT bitmap size >> >> Again, some structure with a size field would be better from >> UAPI/extensibility standpoint. >> >> One additional point: "size" in the structure from kernel should have >> structure size expected by kernel, and at least providing there "0" from >> user space shouldn't lead to failure (in fact, it is possible to provide >> structure size back to userspace even if buffer is too small, along >> with error). > > This has been in GLIBC v2.28. We cannot change it anymore. In theory, you could, if you change the ARCH_CET_LEGACY_BITMAP constant, so that glibc will not use the different arch_prctl operation. We could backport the change into the glibc 2.28 dynamic linker, so that existing binaries will start using CET again. Then only statically linked binaries will be impacted. It's definitely not ideal, but it's doable if the interface is terminally broken or otherwise unacceptable. But to me it looks like this threshold isn't reached here.
> On Oct 4, 2018, at 8:37 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > >> On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote: >>> On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote: >>> Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect >>> Branch Tracking features. >>> >>> Introduce: >>> >>> arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr) >>> Enable the Indirect Branch Tracking legacy code bitmap. >>> >>> The parameter 'addr' is a pointer to a user buffer. >>> On returning to the caller, the kernel fills the following: >>> >>> *addr = IBT bitmap base address >>> *(addr + 1) = IBT bitmap size >> >> Again, some structure with a size field would be better from >> UAPI/extensibility standpoint. >> >> One additional point: "size" in the structure from kernel should have >> structure size expected by kernel, and at least providing there "0" from >> user space shouldn't lead to failure (in fact, it is possible to provide >> structure size back to userspace even if buffer is too small, along >> with error). > > This has been in GLIBC v2.28. We cannot change it anymore. Sure you can. Just change ARCH_CET_LEGACY_BITMAP to a new number. You might need to change all the constants. And if the ELF note by itself causes a problem too, you may need to rename it. And maybe ask glibc to kindly not enable code that depends on non-upstreamed kernel features. There is not, and has never been, any ABI compatibility requirement that says that, if glibc 2.28 "enables" a feature, that the kernel will ever enable it in a way that makes glibc 2.28 actually support it. All the kernel needs to do is avoid making glibc 2.28 *crash*.
On Thu, Oct 4, 2018 at 9:08 AM Florian Weimer <fw@deneb.enyo.de> wrote: > > * Yu-cheng Yu: > > > On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote: > >> On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote: > >> > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect > >> > Branch Tracking features. > >> > > >> > Introduce: > >> > > >> > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr) > >> > Enable the Indirect Branch Tracking legacy code bitmap. > >> > > >> > The parameter 'addr' is a pointer to a user buffer. > >> > On returning to the caller, the kernel fills the following: > >> > > >> > *addr = IBT bitmap base address > >> > *(addr + 1) = IBT bitmap size > >> > >> Again, some structure with a size field would be better from > >> UAPI/extensibility standpoint. > >> > >> One additional point: "size" in the structure from kernel should have > >> structure size expected by kernel, and at least providing there "0" from > >> user space shouldn't lead to failure (in fact, it is possible to provide > >> structure size back to userspace even if buffer is too small, along > >> with error). > > > > This has been in GLIBC v2.28. We cannot change it anymore. > > In theory, you could, if you change the ARCH_CET_LEGACY_BITMAP > constant, so that glibc will not use the different arch_prctl > operation. We could backport the change into the glibc 2.28 dynamic > linker, so that existing binaries will start using CET again. Then > only statically linked binaries will be impacted. > > It's definitely not ideal, but it's doable if the interface is > terminally broken or otherwise unacceptable. But to me it looks like > this threshold isn't reached here. I tend to agree. But I do think there's a real problem that should be fixed and won't affect ABI: the *name* of the prctl is pretty bad. I read the test several times trying to decide if you meant ARCH_GET_CET_LEGACY_BITMAP? But you don't. Maybe name it ARCH_CET_CREATE_LEGACY_BITMAP? And explicitly document what it does if legacy bitmap already exists? --Andy
On Thu, 2018-10-04 at 09:12 -0700, Andy Lutomirski wrote: > On Thu, Oct 4, 2018 at 9:08 AM Florian Weimer <fw@deneb.enyo.de> wrote: > > > > * Yu-cheng Yu: > > > > > On Thu, 2018-10-04 at 15:28 +0200, Eugene Syromiatnikov wrote: > > > > On Fri, Sep 21, 2018 at 08:05:50AM -0700, Yu-cheng Yu wrote: > > > > > Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect > > > > > Branch Tracking features. > > > > > > > > > > Introduce: > > > > > > > > > > arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr) > > > > > Enable the Indirect Branch Tracking legacy code bitmap. > > > > > > > > > > The parameter 'addr' is a pointer to a user buffer. > > > > > On returning to the caller, the kernel fills the following: > > > > > > > > > > *addr = IBT bitmap base address > > > > > *(addr + 1) = IBT bitmap size > > > > > > > > Again, some structure with a size field would be better from > > > > UAPI/extensibility standpoint. > > > > > > > > One additional point: "size" in the structure from kernel should have > > > > structure size expected by kernel, and at least providing there "0" from > > > > user space shouldn't lead to failure (in fact, it is possible to provide > > > > structure size back to userspace even if buffer is too small, along > > > > with error). > > > > > > This has been in GLIBC v2.28. We cannot change it anymore. > > > > In theory, you could, if you change the ARCH_CET_LEGACY_BITMAP > > constant, so that glibc will not use the different arch_prctl > > operation. We could backport the change into the glibc 2.28 dynamic > > linker, so that existing binaries will start using CET again. Then > > only statically linked binaries will be impacted. > > > > It's definitely not ideal, but it's doable if the interface is > > terminally broken or otherwise unacceptable. But to me it looks like > > this threshold isn't reached here. > > I tend to agree. > > But I do think there's a real problem that should be fixed and won't > affect ABI: the *name* of the prctl is pretty bad. I read the test > several times trying to decide if you meant > ARCH_GET_CET_LEGACY_BITMAP? But you don't. > > Maybe name it ARCH_CET_CREATE_LEGACY_BITMAP? And explicitly document > what it does if legacy bitmap already exists? I will fix it. Yu-cheng
diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h index 3aec1088e01d..31d2465f9caf 100644 --- a/arch/x86/include/uapi/asm/prctl.h +++ b/arch/x86/include/uapi/asm/prctl.h @@ -18,5 +18,6 @@ #define ARCH_CET_DISABLE 0x3002 #define ARCH_CET_LOCK 0x3003 #define ARCH_CET_ALLOC_SHSTK 0x3004 +#define ARCH_CET_LEGACY_BITMAP 0x3005 #endif /* _ASM_X86_PRCTL_H */ diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c index c4b7c19f5040..df47b5ebc3f4 100644 --- a/arch/x86/kernel/cet_prctl.c +++ b/arch/x86/kernel/cet_prctl.c @@ -20,6 +20,8 @@ static int handle_get_status(unsigned long arg2) if (current->thread.cet.shstk_enabled) features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK; + if (current->thread.cet.ibt_enabled) + features |= GNU_PROPERTY_X86_FEATURE_1_IBT; shstk_base = current->thread.cet.shstk_base; shstk_size = current->thread.cet.shstk_size; @@ -49,9 +51,35 @@ static int handle_alloc_shstk(unsigned long arg2) return 0; } +static int handle_bitmap(unsigned long arg2) +{ + unsigned long addr, size; + + if (current->thread.cet.ibt_enabled) { + int err; + + err = cet_setup_ibt_bitmap(); + if (err) + return err; + + addr = current->thread.cet.ibt_bitmap_addr; + size = current->thread.cet.ibt_bitmap_size; + } else { + addr = 0; + size = 0; + } + + if (put_user(addr, (unsigned long __user *)arg2) || + put_user(size, (unsigned long __user *)arg2 + 1)) + return -EFAULT; + + return 0; +} + int prctl_cet(int option, unsigned long arg2) { - if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) && + !cpu_feature_enabled(X86_FEATURE_IBT)) return -EINVAL; switch (option) { @@ -63,6 +91,8 @@ int prctl_cet(int option, unsigned long arg2) return -EPERM; if (arg2 & GNU_PROPERTY_X86_FEATURE_1_SHSTK) cet_disable_free_shstk(current); + if (arg2 & GNU_PROPERTY_X86_FEATURE_1_IBT) + cet_disable_ibt(); return 0; @@ -73,6 +103,12 @@ int prctl_cet(int option, unsigned long arg2) case ARCH_CET_ALLOC_SHSTK: return handle_alloc_shstk(arg2); + /* + * Allocate legacy bitmap and return address & size to user. + */ + case ARCH_CET_LEGACY_BITMAP: + return handle_bitmap(arg2); + default: return -EINVAL; } diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index ac0ea9c7e89f..aea15a9b6a3e 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -797,6 +797,7 @@ long do_arch_prctl_common(struct task_struct *task, int option, case ARCH_CET_DISABLE: case ARCH_CET_LOCK: case ARCH_CET_ALLOC_SHSTK: + case ARCH_CET_LEGACY_BITMAP: return prctl_cet(option, cpuid_enabled); }
Update ARCH_CET_STATUS and ARCH_CET_DISABLE to include Indirect Branch Tracking features. Introduce: arch_prctl(ARCH_CET_LEGACY_BITMAP, unsigned long *addr) Enable the Indirect Branch Tracking legacy code bitmap. The parameter 'addr' is a pointer to a user buffer. On returning to the caller, the kernel fills the following: *addr = IBT bitmap base address *(addr + 1) = IBT bitmap size Signed-off-by: H.J. Lu <hjl.tools@gmail.com> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/include/uapi/asm/prctl.h | 1 + arch/x86/kernel/cet_prctl.c | 38 ++++++++++++++++++++++++++++++- arch/x86/kernel/process.c | 1 + 3 files changed, 39 insertions(+), 1 deletion(-)