| Message ID | 20200604134957.505389-6-alex.popov@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improvements of the stackleak gcc plugin | expand |
On Thu, Jun 04, 2020 at 04:49:57PM +0300, Alexander Popov wrote: > Don't try instrumenting functions in arch/arm64/kernel/vdso/vgettimeofday.c. > Otherwise that can cause issues if the cleanup pass of stackleak gcc plugin > is disabled. > > Signed-off-by: Alexander Popov <alex.popov@linux.com> > --- > arch/arm64/kernel/vdso/Makefile | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile > index 3862cad2410c..9b84cafbd2da 100644 > --- a/arch/arm64/kernel/vdso/Makefile > +++ b/arch/arm64/kernel/vdso/Makefile > @@ -32,7 +32,8 @@ UBSAN_SANITIZE := n > OBJECT_FILES_NON_STANDARD := y > KCOV_INSTRUMENT := n > > -CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables > +CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables \ > + $(DISABLE_STACKLEAK_PLUGIN) I can pick this one up via arm64, thanks. Are there any other plugins we should be wary of? It looks like x86 filters out $(GCC_PLUGINS_CFLAGS) when building the vDSO. Will
On Thu, Jun 4, 2020 at 3:58 PM Will Deacon <will@kernel.org> wrote: > On Thu, Jun 04, 2020 at 04:49:57PM +0300, Alexander Popov wrote: > > Don't try instrumenting functions in arch/arm64/kernel/vdso/vgettimeofday.c. > > Otherwise that can cause issues if the cleanup pass of stackleak gcc plugin > > is disabled. > > > > Signed-off-by: Alexander Popov <alex.popov@linux.com> > > --- > > arch/arm64/kernel/vdso/Makefile | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile > > index 3862cad2410c..9b84cafbd2da 100644 > > --- a/arch/arm64/kernel/vdso/Makefile > > +++ b/arch/arm64/kernel/vdso/Makefile > > @@ -32,7 +32,8 @@ UBSAN_SANITIZE := n > > OBJECT_FILES_NON_STANDARD := y > > KCOV_INSTRUMENT := n > > > > -CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables > > +CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables \ > > + $(DISABLE_STACKLEAK_PLUGIN) > > I can pick this one up via arm64, thanks. Are there any other plugins we > should be wary of? It looks like x86 filters out $(GCC_PLUGINS_CFLAGS) > when building the vDSO. Maybe at some point we should replace exclusions based on GCC_PLUGINS_CFLAGS and KASAN_SANITIZE and UBSAN_SANITIZE and OBJECT_FILES_NON_STANDARD and so on with something more generic... something that says "this file will not be built into the normal kernel, it contains code that runs in realmode / userspace / some similarly weird context, and none of our instrumentation infrastructure is available there"...
On 04.06.2020 17:14, Jann Horn wrote: > On Thu, Jun 4, 2020 at 3:58 PM Will Deacon <will@kernel.org> wrote: >> On Thu, Jun 04, 2020 at 04:49:57PM +0300, Alexander Popov wrote: >>> Don't try instrumenting functions in arch/arm64/kernel/vdso/vgettimeofday.c. >>> Otherwise that can cause issues if the cleanup pass of stackleak gcc plugin >>> is disabled. >>> >>> Signed-off-by: Alexander Popov <alex.popov@linux.com> >>> --- >>> arch/arm64/kernel/vdso/Makefile | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile >>> index 3862cad2410c..9b84cafbd2da 100644 >>> --- a/arch/arm64/kernel/vdso/Makefile >>> +++ b/arch/arm64/kernel/vdso/Makefile >>> @@ -32,7 +32,8 @@ UBSAN_SANITIZE := n >>> OBJECT_FILES_NON_STANDARD := y >>> KCOV_INSTRUMENT := n >>> >>> -CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables >>> +CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables \ >>> + $(DISABLE_STACKLEAK_PLUGIN) >> >> I can pick this one up via arm64, thanks. Are there any other plugins we >> should be wary of? I can't tell exactly. I'm sure Kees has the whole picture. >> It looks like x86 filters out $(GCC_PLUGINS_CFLAGS) >> when building the vDSO. Yes, that's why building x86 vDSO doesn't need $(DISABLE_STACKLEAK_PLUGIN). > Maybe at some point we should replace exclusions based on > GCC_PLUGINS_CFLAGS and KASAN_SANITIZE and UBSAN_SANITIZE and > OBJECT_FILES_NON_STANDARD and so on with something more generic... > something that says "this file will not be built into the normal > kernel, it contains code that runs in realmode / userspace / some > similarly weird context, and none of our instrumentation > infrastructure is available there"... Good idea. I would also add 'notrace' to that list. Best regards, Alexander
On Thu, Jun 4, 2020 at 4:21 PM Alexander Popov <alex.popov@linux.com> wrote: > On 04.06.2020 17:14, Jann Horn wrote: > > Maybe at some point we should replace exclusions based on > > GCC_PLUGINS_CFLAGS and KASAN_SANITIZE and UBSAN_SANITIZE and > > OBJECT_FILES_NON_STANDARD and so on with something more generic... > > something that says "this file will not be built into the normal > > kernel, it contains code that runs in realmode / userspace / some > > similarly weird context, and none of our instrumentation > > infrastructure is available there"... > > Good idea. I would also add 'notrace' to that list. Hm? notrace code should definitely still be subject to sanitizer instrumentation.
On 04.06.2020 17:25, Jann Horn wrote: > On Thu, Jun 4, 2020 at 4:21 PM Alexander Popov <alex.popov@linux.com> wrote: >> On 04.06.2020 17:14, Jann Horn wrote: >>> Maybe at some point we should replace exclusions based on >>> GCC_PLUGINS_CFLAGS and KASAN_SANITIZE and UBSAN_SANITIZE and >>> OBJECT_FILES_NON_STANDARD and so on with something more generic... >>> something that says "this file will not be built into the normal >>> kernel, it contains code that runs in realmode / userspace / some >>> similarly weird context, and none of our instrumentation >>> infrastructure is available there"... >> >> Good idea. I would also add 'notrace' to that list. > > Hm? notrace code should definitely still be subject to sanitizer > instrumentation. I mean ftrace is sometimes disabled for functions that are executed in those weird contexts. As well as kcov instrumentation. It would be nice if that generic mechanism could help with choosing which kernel code instrumentation technologies should be disabled in the given context. Best regards, Alexander
On Thu, Jun 04, 2020 at 02:58:06PM +0100, Will Deacon wrote: > On Thu, Jun 04, 2020 at 04:49:57PM +0300, Alexander Popov wrote: > > Don't try instrumenting functions in arch/arm64/kernel/vdso/vgettimeofday.c. > > Otherwise that can cause issues if the cleanup pass of stackleak gcc plugin > > is disabled. > > > > Signed-off-by: Alexander Popov <alex.popov@linux.com> > > --- > > arch/arm64/kernel/vdso/Makefile | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile > > index 3862cad2410c..9b84cafbd2da 100644 > > --- a/arch/arm64/kernel/vdso/Makefile > > +++ b/arch/arm64/kernel/vdso/Makefile > > @@ -32,7 +32,8 @@ UBSAN_SANITIZE := n > > OBJECT_FILES_NON_STANDARD := y > > KCOV_INSTRUMENT := n > > > > -CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables > > +CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables \ > > + $(DISABLE_STACKLEAK_PLUGIN) > > I can pick this one up via arm64, thanks. Are there any other plugins we > should be wary of? It looks like x86 filters out $(GCC_PLUGINS_CFLAGS) > when building the vDSO. I didn't realize/remember that arm64 retained the kernel build flags for vDSO builds. (I'm used to x86 throwing all its flags away for its vDSO.) How does 32-bit ARM do its vDSO? My quick run-through on plugins: arm_ssp_per_task_plugin.c 32-bit ARM only (but likely needs disabling for 32-bit ARM vDSO?) cyc_complexity_plugin.c compile-time reporting only latent_entropy_plugin.c this shouldn't get triggered for the vDSO (no __latent_entropy nor __init attributes in vDSO), but perhaps explicitly disabling it would be a sensible thing to do, just for robustness? randomize_layout_plugin.c this shouldn't get triggered (again, lacking attributes), but should likely be disabled too. sancov_plugin.c This should be tracking the KCOV directly (see scripts/Makefile.kcov), which is already disabled here. structleak_plugin.c This should be fine in the vDSO, but there's not security boundary here, so it wouldn't be important to KEEP it enabled.
On Tue, Jun 09, 2020 at 12:09:27PM -0700, Kees Cook wrote: > On Thu, Jun 04, 2020 at 02:58:06PM +0100, Will Deacon wrote: > > On Thu, Jun 04, 2020 at 04:49:57PM +0300, Alexander Popov wrote: > > > Don't try instrumenting functions in arch/arm64/kernel/vdso/vgettimeofday.c. > > > Otherwise that can cause issues if the cleanup pass of stackleak gcc plugin > > > is disabled. > > > > > > Signed-off-by: Alexander Popov <alex.popov@linux.com> > > > --- > > > arch/arm64/kernel/vdso/Makefile | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile > > > index 3862cad2410c..9b84cafbd2da 100644 > > > --- a/arch/arm64/kernel/vdso/Makefile > > > +++ b/arch/arm64/kernel/vdso/Makefile > > > @@ -32,7 +32,8 @@ UBSAN_SANITIZE := n > > > OBJECT_FILES_NON_STANDARD := y > > > KCOV_INSTRUMENT := n > > > > > > -CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables > > > +CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables \ > > > + $(DISABLE_STACKLEAK_PLUGIN) > > > > I can pick this one up via arm64, thanks. Are there any other plugins we > > should be wary of? It looks like x86 filters out $(GCC_PLUGINS_CFLAGS) > > when building the vDSO. > > I didn't realize/remember that arm64 retained the kernel build flags for > vDSO builds. (I'm used to x86 throwing all its flags away for its vDSO.) > > How does 32-bit ARM do its vDSO? > > My quick run-through on plugins: > > arm_ssp_per_task_plugin.c > 32-bit ARM only (but likely needs disabling for 32-bit ARM vDSO?) On arm64, the 32-bit toolchain is picked up via CC_COMPAT -- does that still get the plugins? > cyc_complexity_plugin.c > compile-time reporting only > > latent_entropy_plugin.c > this shouldn't get triggered for the vDSO (no __latent_entropy > nor __init attributes in vDSO), but perhaps explicitly disabling > it would be a sensible thing to do, just for robustness? > > randomize_layout_plugin.c > this shouldn't get triggered (again, lacking attributes), but > should likely be disabled too. > > sancov_plugin.c > This should be tracking the KCOV directly (see > scripts/Makefile.kcov), which is already disabled here. > > structleak_plugin.c > This should be fine in the vDSO, but there's not security > boundary here, so it wouldn't be important to KEEP it enabled. Thanks for going through these. In general though, it seems like an opt-in strategy would make more sense, as it doesn't make an awful lot of sense to me for the plugins to be used to build the vDSO. So I would prefer that this patch filters out $(GCC_PLUGINS_CFLAGS). Will
On 10.06.2020 10:30, Will Deacon wrote: > On Tue, Jun 09, 2020 at 12:09:27PM -0700, Kees Cook wrote: >> On Thu, Jun 04, 2020 at 02:58:06PM +0100, Will Deacon wrote: >>> On Thu, Jun 04, 2020 at 04:49:57PM +0300, Alexander Popov wrote: >>>> Don't try instrumenting functions in arch/arm64/kernel/vdso/vgettimeofday.c. >>>> Otherwise that can cause issues if the cleanup pass of stackleak gcc plugin >>>> is disabled. >>>> >>>> Signed-off-by: Alexander Popov <alex.popov@linux.com> >>>> --- >>>> arch/arm64/kernel/vdso/Makefile | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile >>>> index 3862cad2410c..9b84cafbd2da 100644 >>>> --- a/arch/arm64/kernel/vdso/Makefile >>>> +++ b/arch/arm64/kernel/vdso/Makefile >>>> @@ -32,7 +32,8 @@ UBSAN_SANITIZE := n >>>> OBJECT_FILES_NON_STANDARD := y >>>> KCOV_INSTRUMENT := n >>>> >>>> -CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables >>>> +CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables \ >>>> + $(DISABLE_STACKLEAK_PLUGIN) >>> >>> I can pick this one up via arm64, thanks. Are there any other plugins we >>> should be wary of? It looks like x86 filters out $(GCC_PLUGINS_CFLAGS) >>> when building the vDSO. >> >> I didn't realize/remember that arm64 retained the kernel build flags for >> vDSO builds. (I'm used to x86 throwing all its flags away for its vDSO.) >> >> How does 32-bit ARM do its vDSO? >> >> My quick run-through on plugins: >> >> arm_ssp_per_task_plugin.c >> 32-bit ARM only (but likely needs disabling for 32-bit ARM vDSO?) > > On arm64, the 32-bit toolchain is picked up via CC_COMPAT -- does that still > get the plugins? > >> cyc_complexity_plugin.c >> compile-time reporting only >> >> latent_entropy_plugin.c >> this shouldn't get triggered for the vDSO (no __latent_entropy >> nor __init attributes in vDSO), but perhaps explicitly disabling >> it would be a sensible thing to do, just for robustness? >> >> randomize_layout_plugin.c >> this shouldn't get triggered (again, lacking attributes), but >> should likely be disabled too. >> >> sancov_plugin.c >> This should be tracking the KCOV directly (see >> scripts/Makefile.kcov), which is already disabled here. >> >> structleak_plugin.c >> This should be fine in the vDSO, but there's not security >> boundary here, so it wouldn't be important to KEEP it enabled. > > Thanks for going through these. In general though, it seems like an > opt-in strategy would make more sense, as it doesn't make an awful lot > of sense to me for the plugins to be used to build the vDSO. > > So I would prefer that this patch filters out $(GCC_PLUGINS_CFLAGS). Ok, I will do that in the v2 of the patch series. Best regards, Alexander
On 10.06.2020 10:30, Will Deacon wrote: > On Tue, Jun 09, 2020 at 12:09:27PM -0700, Kees Cook wrote: >> arm_ssp_per_task_plugin.c >> 32-bit ARM only (but likely needs disabling for 32-bit ARM vDSO?) I tested: on 32-bit arm vDSO is built with plugin flags. I will filter them out in a separate patch in v2 of the series. > On arm64, the 32-bit toolchain is picked up via CC_COMPAT -- does that still > get the plugins? I tested it with this command: make ARCH=arm64 CROSS_COMPILE_COMPAT=arm-linux-gnueabi- CROSS_COMPILE=aarch64-linux-gnu- V=1 I see that COMPAT_VDSO is built without plugin flags. So it's ok. Best regards, Alexander
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile index 3862cad2410c..9b84cafbd2da 100644 --- a/arch/arm64/kernel/vdso/Makefile +++ b/arch/arm64/kernel/vdso/Makefile @@ -32,7 +32,8 @@ UBSAN_SANITIZE := n OBJECT_FILES_NON_STANDARD := y KCOV_INSTRUMENT := n -CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables +CFLAGS_vgettimeofday.o = -O2 -mcmodel=tiny -fasynchronous-unwind-tables \ + $(DISABLE_STACKLEAK_PLUGIN) ifneq ($(c-gettimeofday-y),) CFLAGS_vgettimeofday.o += -include $(c-gettimeofday-y)
Don't try instrumenting functions in arch/arm64/kernel/vdso/vgettimeofday.c. Otherwise that can cause issues if the cleanup pass of stackleak gcc plugin is disabled. Signed-off-by: Alexander Popov <alex.popov@linux.com> --- arch/arm64/kernel/vdso/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)