[PATCH 0/4] KASAN/ARM64 EFI fixes

[PATCH 0/4] KASAN/ARM64 EFI fixes

[Andrey Ryabinin]

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

[PATCH 1/4] x86, efi, kasan: #undef memset/memcpy/memmove per arch.

[Andrey Ryabinin]

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 <ryabinin.a.a@gmail.com>
---
 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

[PATCH 1/4] x86, efi, kasan: #undef memset/memcpy/memmove per arch.

[Ingo Molnar]

* Andrey Ryabinin <ryabinin.a.a@gmail.com> 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 <ryabinin.a.a@gmail.com>
> ---
>  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

[PATCH 1/4] x86, efi, kasan: #undef memset/memcpy/memmove per arch.

[Leif Lindholm]

On Fri, Aug 28, 2015 at 08:27:45AM +0200, Ingo Molnar wrote:
> 
> * Andrey Ryabinin <ryabinin.a.a@gmail.com> 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 <ryabinin.a.a@gmail.com>
> > ---
> >  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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] x86, efi, kasan: #undef memset/memcpy/memmove per arch.

[Ingo Molnar]

* Leif Lindholm <leif.lindholm@linaro.org> 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

[PATCH v2] x86, efi, kasan: #undef memset/memcpy/memmove per arch.

[Andrey Ryabinin]

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 <ryabinin.a.a@gmail.com>
---
 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

[PATCH v2] x86, efi, kasan: #undef memset/memcpy/memmove per arch.

[Matt Fleming]

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 <ryabinin.a.a@gmail.com>
> ---
>  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

[PATCH v2] x86, efi, kasan: #undef memset/memcpy/memmove per arch.

[Andrey Ryabinin]

2015-08-28 17:43 GMT+03:00 Matt Fleming <matt@codeblueprint.co.uk>:
> 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 <ryabinin.a.a@gmail.com>
>> ---
>>  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

[PATCH 2/4] efi/runtime-wrappers, kasan: don't sanitize runtime wrappers.

[Andrey Ryabinin]

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 <ryabinin.a.a@gmail.com>
---
 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

[PATCH 3/4] arm64/efi-stub, kasan: don't instrument efi-stub.

[Andrey Ryabinin]

efi-stub.c code runs before we setup early shadow.
Intrumentation of it will cause boot crash.

Signed-off-by: Andrey Ryabinin <ryabinin.a.a@gmail.com>
---
 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

[PATCH 4/4] libfdt, kasan: don't instrument libfdt.

[Andrey Ryabinin]

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 <ryabinin.a.a@gmail.com>
---
 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