On 08/13/2015 08:23 PM, Will Deacon wrote:
> I tried this out under EFI and it dies horribly in the stub code because
> we're missing at least one KASAN_SANITIZE_ Makefile entry.
>
> So I think this needs longer to stew before hitting mainline. By all means
> get the x86 dependencies in for 4.3, but the arm64 port can probably use
> another cycle to iron out the bugs.
>
> Cheers,
>
> Will
>
These series fixes boot crash with KASAN under EFI.
I'm sending it separetly for easier review.
We have one more small x86 dependcy in the first patch.
Patches 2-4 I'm going to squash into v6 'arm64: add KASAN support' patch.
Andrey Ryabinin (4):
x86, efi, kasan: #undef memset/memcpy/memmove per arch.
efi/runtime-wrappers, kasan: don't sanitize runtime wrappers.
arm64/efi-stub, kasan: don't instrument efi-stub.
libfdt, kasan: don't instrument libfdt.
arch/arm64/kernel/Makefile | 2 ++
arch/x86/include/asm/efi.h | 8 ++++++++
drivers/firmware/efi/Makefile | 8 ++++++++
drivers/firmware/efi/libstub/efistub.h | 4 ----
lib/Makefile | 3 ++-
5 files changed, 20 insertions(+), 5 deletions(-)
--
2.4.6
In not-instrumented code KASAN replaces instrumented
memset/memcpy/memmove with not-instrumented analogues
__memset/__memcpy/__memove.
However, on x86 EFI stub is not linked with kernel.
It uses not-instrumented mem*() functions from
arch/x86/boot/compressed/string.c
So we don't replace them with __mem*() variants in EFI stub.
In ARM64 EFI stub is linked with the kernel, so we should
replace mem*() functions with __mem*(), because EFI stub
runs before KASAN setup early shadow.
So let's move these #undef mem* into arch's asm/efi.h which is
also included by EFI stub.
Signed-off-by: Andrey Ryabinin <[email protected]>
---
arch/x86/include/asm/efi.h | 8 ++++++++
drivers/firmware/efi/libstub/efistub.h | 4 ----
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 155162e..d821ce2 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -25,6 +25,14 @@
#define EFI32_LOADER_SIGNATURE "EL32"
#define EFI64_LOADER_SIGNATURE "EL64"
+/*
+ * Use memcpy/memset from arch/x86/boot/compressed/string.c
+ * for EFI stub.
+ */
+#undef memcpy
+#undef memset
+#undef memmove
+
#ifdef CONFIG_X86_32
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index e334a01..6b6548f 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -5,10 +5,6 @@
/* error code which can't be mistaken for valid address */
#define EFI_ERROR (~0UL)
-#undef memcpy
-#undef memset
-#undef memmove
-
void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, void *__image,
--
2.4.6
ARM64 maps efi runtime services in userspace addresses
which don't have KASAN shadow. So dereferencing these addresses
in efi_call_virt() will cause crash if this code instrumented.
Signed-off-by: Andrey Ryabinin <[email protected]>
---
drivers/firmware/efi/Makefile | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 6fd3da9..413fcf2 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,6 +1,14 @@
#
# Makefile for linux kernel
#
+
+#
+# ARM64 maps efi runtime services in userspace addresses
+# which don't have KASAN shadow. So dereference of these addresses
+# in efi_call_virt() will cause crash if this code instrumented.
+#
+KASAN_SANITIZE_runtime-wrappers.o := n
+
obj-$(CONFIG_EFI) += efi.o vars.o reboot.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_ESRT) += esrt.o
--
2.4.6
efi-stub.c code runs before we setup early shadow.
Intrumentation of it will cause boot crash.
Signed-off-by: Andrey Ryabinin <[email protected]>
---
arch/arm64/kernel/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 426d076..086b122 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -7,6 +7,8 @@ AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
CFLAGS_armv8_deprecated.o := -I$(src)
+KASAN_SANITIZE_efi-stub.o := n
+
CFLAGS_REMOVE_ftrace.o = -pg
CFLAGS_REMOVE_insn.o = -pg
CFLAGS_REMOVE_return_address.o = -pg
--
2.4.6
libfdt is used by EFI code and it runs before we setup early
shadow, so it has to be not instrumented to prevent boot crash.
Signed-off-by: Andrey Ryabinin <[email protected]>
---
lib/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/Makefile b/lib/Makefile
index 6897b52..44e1d81 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -165,7 +165,8 @@ obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
fdt_empty_tree.o
$(foreach file, $(libfdt_files), \
- $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt))
+ $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt) \
+ $(eval KASAN_SANITIZE_$(file) := n))
lib-$(CONFIG_LIBFDT) += $(libfdt_files)
obj-$(CONFIG_RBTREE_TEST) += rbtree_test.o
--
2.4.6
* Andrey Ryabinin <[email protected]> wrote:
> In not-instrumented code KASAN replaces instrumented
> memset/memcpy/memmove with not-instrumented analogues
> __memset/__memcpy/__memove.
> However, on x86 EFI stub is not linked with kernel.
s/with the kernel
> It uses not-instrumented mem*() functions from
> arch/x86/boot/compressed/string.c
> So we don't replace them with __mem*() variants in EFI stub.
>
> In ARM64 EFI stub is linked with the kernel, so we should
s/In ARM64 EFI stub/On ARM64 the EFI stub/.
> replace mem*() functions with __mem*(), because EFI stub
s/the EFI stub
> runs before KASAN setup early shadow.
s/before KASAN sets up the early shadow
?
> So let's move these #undef mem* into arch's asm/efi.h which is
s/lets
> also included by EFI stub.
s/by the EFI stub
>
> Signed-off-by: Andrey Ryabinin <[email protected]>
> ---
> arch/x86/include/asm/efi.h | 8 ++++++++
> drivers/firmware/efi/libstub/efistub.h | 4 ----
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 155162e..d821ce2 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -25,6 +25,14 @@
> #define EFI32_LOADER_SIGNATURE "EL32"
> #define EFI64_LOADER_SIGNATURE "EL64"
>
> +/*
> + * Use memcpy/memset from arch/x86/boot/compressed/string.c
> + * for EFI stub.
> + */
> +#undef memcpy
> +#undef memset
> +#undef memmove
This comment should also mention why it's done, not what is done.
Also:
s/for EFI stub/for the EFI stub
Thanks,
Ingo
On Fri, Aug 28, 2015 at 08:27:45AM +0200, Ingo Molnar wrote:
>
> * Andrey Ryabinin <[email protected]> wrote:
>
> > In not-instrumented code KASAN replaces instrumented
> > memset/memcpy/memmove with not-instrumented analogues
> > __memset/__memcpy/__memove.
> > However, on x86 EFI stub is not linked with kernel.
>
> s/with the kernel
>
> > It uses not-instrumented mem*() functions from
> > arch/x86/boot/compressed/string.c
> > So we don't replace them with __mem*() variants in EFI stub.
> >
> > In ARM64 EFI stub is linked with the kernel, so we should
>
> s/In ARM64 EFI stub/On ARM64 the EFI stub/.
>
> > replace mem*() functions with __mem*(), because EFI stub
>
> s/the EFI stub
>
> > runs before KASAN setup early shadow.
>
> s/before KASAN sets up the early shadow
>
> ?
>
> > So let's move these #undef mem* into arch's asm/efi.h which is
>
> s/lets
No, that one is actually correct. Abbreviation of let us.
> > also included by EFI stub.
>
> s/by the EFI stub
>
> >
> > Signed-off-by: Andrey Ryabinin <[email protected]>
> > ---
> > arch/x86/include/asm/efi.h | 8 ++++++++
> > drivers/firmware/efi/libstub/efistub.h | 4 ----
> > 2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> > index 155162e..d821ce2 100644
> > --- a/arch/x86/include/asm/efi.h
> > +++ b/arch/x86/include/asm/efi.h
> > @@ -25,6 +25,14 @@
> > #define EFI32_LOADER_SIGNATURE "EL32"
> > #define EFI64_LOADER_SIGNATURE "EL64"
> >
> > +/*
> > + * Use memcpy/memset from arch/x86/boot/compressed/string.c
> > + * for EFI stub.
> > + */
> > +#undef memcpy
> > +#undef memset
> > +#undef memmove
>
> This comment should also mention why it's done, not what is done.
>
> Also:
>
> s/for EFI stub/for the EFI stub
>
> Thanks,
>
> Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
* Leif Lindholm <[email protected]> wrote:
> > > So let's move these #undef mem* into arch's asm/efi.h which is
> >
> > s/lets
>
> No, that one is actually correct. Abbreviation of let us.
Indeed.
Thanks,
Ingo
In not-instrumented code KASAN replaces instrumented
memset/memcpy/memmove with not-instrumented analogues
__memset/__memcpy/__memove.
However, on x86 the EFI stub is not linked with the kernel.
It uses not-instrumented mem*() functions from
arch/x86/boot/compressed/string.c
So we don't replace them with __mem*() variants in EFI stub.
On ARM64 the EFI stub is linked with the kernel, so we should
replace mem*() functions with __mem*(), because the EFI stub
runs before KASAN sets up early shadow.
So let's move these #undef mem* into arch's asm/efi.h which is
also included by the EFI stub.
Signed-off-by: Andrey Ryabinin <[email protected]>
---
arch/x86/include/asm/efi.h | 11 +++++++++++
drivers/firmware/efi/libstub/efistub.h | 4 ----
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 155162e..6862f11 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -25,6 +25,17 @@
#define EFI32_LOADER_SIGNATURE "EL32"
#define EFI64_LOADER_SIGNATURE "EL64"
+/*
+ * Sometimes we may redefine memset to __memset.
+ * The EFI stub doesn't have __memset() because it's not
+ * linked with the kernel. So we should use standard
+ * memset from arch/x86/boot/compressed/string.c
+ * The same applies to memcpy and memmove.
+ */
+#undef memcpy
+#undef memset
+#undef memmove
+
#ifdef CONFIG_X86_32
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index e334a01..6b6548f 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -5,10 +5,6 @@
/* error code which can't be mistaken for valid address */
#define EFI_ERROR (~0UL)
-#undef memcpy
-#undef memset
-#undef memmove
-
void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, void *__image,
--
2.4.6
On Fri, 28 Aug, at 02:09:59PM, Andrey Ryabinin wrote:
> In not-instrumented code KASAN replaces instrumented
> memset/memcpy/memmove with not-instrumented analogues
> __memset/__memcpy/__memove.
> However, on x86 the EFI stub is not linked with the kernel.
> It uses not-instrumented mem*() functions from
> arch/x86/boot/compressed/string.c
> So we don't replace them with __mem*() variants in EFI stub.
>
> On ARM64 the EFI stub is linked with the kernel, so we should
> replace mem*() functions with __mem*(), because the EFI stub
> runs before KASAN sets up early shadow.
>
> So let's move these #undef mem* into arch's asm/efi.h which is
> also included by the EFI stub.
>
> Signed-off-by: Andrey Ryabinin <[email protected]>
> ---
> arch/x86/include/asm/efi.h | 11 +++++++++++
> drivers/firmware/efi/libstub/efistub.h | 4 ----
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 155162e..6862f11 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -25,6 +25,17 @@
> #define EFI32_LOADER_SIGNATURE "EL32"
> #define EFI64_LOADER_SIGNATURE "EL64"
>
> +/*
> + * Sometimes we may redefine memset to __memset.
> + * The EFI stub doesn't have __memset() because it's not
> + * linked with the kernel. So we should use standard
> + * memset from arch/x86/boot/compressed/string.c
> + * The same applies to memcpy and memmove.
> + */
> +#undef memcpy
> +#undef memset
> +#undef memmove
> +
I think the key piece of information missing in this comment is that
it's redefined for KASAN, right?
> #ifdef CONFIG_X86_32
>
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index e334a01..6b6548f 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -5,10 +5,6 @@
> /* error code which can't be mistaken for valid address */
> #define EFI_ERROR (~0UL)
>
> -#undef memcpy
> -#undef memset
> -#undef memmove
> -
> void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
>
> efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, void *__image,
Does this patch have any negative consequences for arm64? Can we
safely remove these #undefs without breaking the arm64 EFI stub?
--
Matt Fleming, Intel Open Source Technology Center
2015-08-28 17:43 GMT+03:00 Matt Fleming <[email protected]>:
> On Fri, 28 Aug, at 02:09:59PM, Andrey Ryabinin wrote:
>> In not-instrumented code KASAN replaces instrumented
>> memset/memcpy/memmove with not-instrumented analogues
>> __memset/__memcpy/__memove.
>> However, on x86 the EFI stub is not linked with the kernel.
>> It uses not-instrumented mem*() functions from
>> arch/x86/boot/compressed/string.c
>> So we don't replace them with __mem*() variants in EFI stub.
>>
>> On ARM64 the EFI stub is linked with the kernel, so we should
>> replace mem*() functions with __mem*(), because the EFI stub
>> runs before KASAN sets up early shadow.
>>
>> So let's move these #undef mem* into arch's asm/efi.h which is
>> also included by the EFI stub.
>>
>> Signed-off-by: Andrey Ryabinin <[email protected]>
>> ---
>> arch/x86/include/asm/efi.h | 11 +++++++++++
>> drivers/firmware/efi/libstub/efistub.h | 4 ----
>> 2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
>> index 155162e..6862f11 100644
>> --- a/arch/x86/include/asm/efi.h
>> +++ b/arch/x86/include/asm/efi.h
>> @@ -25,6 +25,17 @@
>> #define EFI32_LOADER_SIGNATURE "EL32"
>> #define EFI64_LOADER_SIGNATURE "EL64"
>>
>> +/*
>> + * Sometimes we may redefine memset to __memset.
>> + * The EFI stub doesn't have __memset() because it's not
>> + * linked with the kernel. So we should use standard
>> + * memset from arch/x86/boot/compressed/string.c
>> + * The same applies to memcpy and memmove.
>> + */
>> +#undef memcpy
>> +#undef memset
>> +#undef memmove
>> +
>
> I think the key piece of information missing in this comment is that
> it's redefined for KASAN, right?
>
Right.
>> #ifdef CONFIG_X86_32
>>
>>
>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>> index e334a01..6b6548f 100644
>> --- a/drivers/firmware/efi/libstub/efistub.h
>> +++ b/drivers/firmware/efi/libstub/efistub.h
>> @@ -5,10 +5,6 @@
>> /* error code which can't be mistaken for valid address */
>> #define EFI_ERROR (~0UL)
>>
>> -#undef memcpy
>> -#undef memset
>> -#undef memmove
>> -
>> void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
>>
>> efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, void *__image,
>
> Does this patch have any negative consequences for arm64? Can we
> safely remove these #undefs without breaking the arm64 EFI stub?
>
The whole point of this patch is to remove these #undefs on ARM64.
Otherwise we end up calling instrumented memset, which will access to
kasan shadow,
before we set it up
So on ARM64 we should use not-instumented __memset on ARM64, iow we
need to keep #define memset __memset.
> --
> Matt Fleming, Intel Open Source Technology Center