Re: [PATCH v7 4/4] powerpc: Book3S 64-bit "heavyweight" KASAN support
Christophe Leroy writes: > Le 13/02/2020 à 01:47, Daniel Axtens a écrit : >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >> index 497b7d0b2d7e..f1c54c08a88e 100644 >> --- a/arch/powerpc/Kconfig >> +++ b/arch/powerpc/Kconfig >> @@ -169,7 +169,9 @@ config PPC >> select HAVE_ARCH_HUGE_VMAP if PPC_BOOK3S_64 && >> PPC_RADIX_MMU >> select HAVE_ARCH_JUMP_LABEL >> select HAVE_ARCH_KASAN if PPC32 >> +select HAVE_ARCH_KASAN if PPC_BOOK3S_64 && >> PPC_RADIX_MMU > > That's probably detail, but as it is necessary to deeply define the HW > when selecting that (I mean giving the exact amount of memory and with > restrictions like having a wholeblock memory), should it also depend of > EXPERT ? If it weren't a debug feature I would definitely agree with you, but I think being a debug feature it's not so necessary. Also anything with more memory than the config option specifies will still boot - it's just less memory that won't boot. I've set the default to 1024MB: I know that's a lot of memory for an embedded system but I think for anything with the Radix MMU it's an OK default. I'm sure if mpe disagrees he can add EXPERT when he's merging :) > Maybe we could have > > - select HAVE_ARCH_KASAN_VMALLOC if PPC32 > + select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN That's a good idea. Done. Thanks for the review! Regards, Daniel > >> select HAVE_ARCH_KGDB >> select HAVE_ARCH_MMAP_RND_BITS >> select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT > > > Christophe
Re: [PATCH v7 4/4] powerpc: Book3S 64-bit "heavyweight" KASAN support
Le 17/02/2020 à 00:08, Michael Neuling a écrit : Daniel. Can you start this commit message with a simple description of what you are actually doing? This reads like you've been on a long journey to Mordor and back, which as a reader of this patch in the long distant future, I don't care about. I just want to know what you're implementing. Also I'm struggling to review this as I don't know what software or hardware mechanisms you are using to perform sanitisation. KASAN is standard, it's simply using GCC ASAN in kernel mode, ie kernel is built with -fsanitize=kernel-address (https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html) You have more details there: https://www.kernel.org/doc/html/latest/dev-tools/kasan.html Christophe
Re: [PATCH v7 4/4] powerpc: Book3S 64-bit "heavyweight" KASAN support
Daniel. Can you start this commit message with a simple description of what you are actually doing? This reads like you've been on a long journey to Mordor and back, which as a reader of this patch in the long distant future, I don't care about. I just want to know what you're implementing. Also I'm struggling to review this as I don't know what software or hardware mechanisms you are using to perform sanitisation. Mikey On Thu, 2020-02-13 at 11:47 +1100, Daniel Axtens wrote: > KASAN support on Book3S is a bit tricky to get right: > > - It would be good to support inline instrumentation so as to be able to >catch stack issues that cannot be caught with outline mode. > > - Inline instrumentation requires a fixed offset. > > - Book3S runs code in real mode after booting. Most notably a lot of KVM >runs in real mode, and it would be good to be able to instrument it. > > - Because code runs in real mode after boot, the offset has to point to >valid memory both in and out of real mode. > > [ppc64 mm note: The kernel installs a linear mapping at effective > address c000... onward. This is a one-to-one mapping with physical > memory from ... onward. Because of how memory accesses work on > powerpc 64-bit Book3S, a kernel pointer in the linear map accesses the > same memory both with translations on (accessing as an 'effective > address'), and with translations off (accessing as a 'real > address'). This works in both guests and the hypervisor. For more > details, see s5.7 of Book III of version 3 of the ISA, in particular > the Storage Control Overview, s5.7.3, and s5.7.5 - noting that this > KASAN implementation currently only supports Radix.] > > One approach is just to give up on inline instrumentation. This way all > checks can be delayed until after everything set is up correctly, and the > address-to-shadow calculations can be overridden. However, the features and > speed boost provided by inline instrumentation are worth trying to do > better. > > If _at compile time_ it is known how much contiguous physical memory a > system has, the top 1/8th of the first block of physical memory can be set > aside for the shadow. This is a big hammer and comes with 3 big > consequences: > > - there's no nice way to handle physically discontiguous memory, so only >the first physical memory block can be used. > > - kernels will simply fail to boot on machines with less memory than >specified when compiling. > > - kernels running on machines with more memory than specified when >compiling will simply ignore the extra memory. > > Implement and document KASAN this way. The current implementation is Radix > only. > > Despite the limitations, it can still find bugs, > e.g. http://patchwork.ozlabs.org/patch/1103775/ > > At the moment, this physical memory limit must be set _even for outline > mode_. This may be changed in a later series - a different implementation > could be added for outline mode that dynamically allocates shadow at a > fixed offset. For example, see https://patchwork.ozlabs.org/patch/795211/ > > Suggested-by: Michael Ellerman > Cc: Balbir Singh # ppc64 out-of-line radix version > Cc: Christophe Leroy # ppc32 version > Signed-off-by: Daniel Axtens > > --- > Changes since v6: > - rework kasan_late_init support, which also fixes book3e problem that > snowpatch >picked up (I think) > - fix a checkpatch error that snowpatch picked up > - don't needlessly move the include in kasan.h > > Changes since v5: > - rebase on powerpc/merge, with Christophe's latest changes integrating >kasan-vmalloc > - documentation tweaks based on latest 32-bit changes > > Changes since v4: > - fix some ppc32 build issues > - support ptdump > - clean up the header file. It turns out we don't need or use > KASAN_SHADOW_SIZE, >so just dump it, and make KASAN_SHADOW_END the thing that varies between 32 >and 64 bit. As part of this, make sure KASAN_SHADOW_OFFSET is only > configured for >32 bit - it is calculated in the Makefile for ppc64. > - various cleanups > > Changes since v3: > - Address further feedback from Christophe. > - Drop changes to stack walking, it looks like the issue I observed is >related to that particular stack, not stack-walking generally. > > Changes since v2: > > - Address feedback from Christophe around cleanups and docs. > - Address feedback from Balbir: at this point I don't have a good solution >for the issues you identify around the limitations of the inline > implementation >but I think that it's worth trying to get the stack instrumentation > support. >I'm happy to have an alternative and more flexible outline mode - I had >envisoned this would be called 'lightweight' mode as it imposes fewer > restrictions. >I've linked to your implementation. I think it's best to add it in a > follow-up series. > - Made the default PHYS_MEM_SIZE_FOR_KASAN value
Re: [PATCH v7 4/4] powerpc: Book3S 64-bit "heavyweight" KASAN support
Le 13/02/2020 à 01:47, Daniel Axtens a écrit : diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 497b7d0b2d7e..f1c54c08a88e 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -169,7 +169,9 @@ config PPC select HAVE_ARCH_HUGE_VMAP if PPC_BOOK3S_64 && PPC_RADIX_MMU select HAVE_ARCH_JUMP_LABEL select HAVE_ARCH_KASAN if PPC32 + select HAVE_ARCH_KASAN if PPC_BOOK3S_64 && PPC_RADIX_MMU That's probably detail, but as it is necessary to deeply define the HW when selecting that (I mean giving the exact amount of memory and with restrictions like having a wholeblock memory), should it also depend of EXPERT ? select HAVE_ARCH_KASAN_VMALLOC if PPC32 + select HAVE_ARCH_KASAN_VMALLOC if PPC_BOOK3S_64 && PPC_RADIX_MMU Maybe we could have - select HAVE_ARCH_KASAN_VMALLOC if PPC32 + select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN select HAVE_ARCH_KGDB select HAVE_ARCH_MMAP_RND_BITS select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT Christophe
Re: [PATCH v7 4/4] powerpc: Book3S 64-bit "heavyweight" KASAN support
Le 13/02/2020 à 01:47, Daniel Axtens a écrit : KASAN support on Book3S is a bit tricky to get right: - It would be good to support inline instrumentation so as to be able to catch stack issues that cannot be caught with outline mode. - Inline instrumentation requires a fixed offset. - Book3S runs code in real mode after booting. Most notably a lot of KVM runs in real mode, and it would be good to be able to instrument it. - Because code runs in real mode after boot, the offset has to point to valid memory both in and out of real mode. [ppc64 mm note: The kernel installs a linear mapping at effective address c000... onward. This is a one-to-one mapping with physical memory from ... onward. Because of how memory accesses work on powerpc 64-bit Book3S, a kernel pointer in the linear map accesses the same memory both with translations on (accessing as an 'effective address'), and with translations off (accessing as a 'real address'). This works in both guests and the hypervisor. For more details, see s5.7 of Book III of version 3 of the ISA, in particular the Storage Control Overview, s5.7.3, and s5.7.5 - noting that this KASAN implementation currently only supports Radix.] One approach is just to give up on inline instrumentation. This way all checks can be delayed until after everything set is up correctly, and the address-to-shadow calculations can be overridden. However, the features and speed boost provided by inline instrumentation are worth trying to do better. If _at compile time_ it is known how much contiguous physical memory a system has, the top 1/8th of the first block of physical memory can be set aside for the shadow. This is a big hammer and comes with 3 big consequences: - there's no nice way to handle physically discontiguous memory, so only the first physical memory block can be used. - kernels will simply fail to boot on machines with less memory than specified when compiling. - kernels running on machines with more memory than specified when compiling will simply ignore the extra memory. Implement and document KASAN this way. The current implementation is Radix only. Despite the limitations, it can still find bugs, e.g. http://patchwork.ozlabs.org/patch/1103775/ At the moment, this physical memory limit must be set _even for outline mode_. This may be changed in a later series - a different implementation could be added for outline mode that dynamically allocates shadow at a fixed offset. For example, see https://patchwork.ozlabs.org/patch/795211/ Suggested-by: Michael Ellerman Cc: Balbir Singh # ppc64 out-of-line radix version Cc: Christophe Leroy # ppc32 version Signed-off-by: Daniel Axtens Reviewed-by: # focussed mainly on Documentation and things impacting PPC32
[PATCH v7 4/4] powerpc: Book3S 64-bit "heavyweight" KASAN support
KASAN support on Book3S is a bit tricky to get right: - It would be good to support inline instrumentation so as to be able to catch stack issues that cannot be caught with outline mode. - Inline instrumentation requires a fixed offset. - Book3S runs code in real mode after booting. Most notably a lot of KVM runs in real mode, and it would be good to be able to instrument it. - Because code runs in real mode after boot, the offset has to point to valid memory both in and out of real mode. [ppc64 mm note: The kernel installs a linear mapping at effective address c000... onward. This is a one-to-one mapping with physical memory from ... onward. Because of how memory accesses work on powerpc 64-bit Book3S, a kernel pointer in the linear map accesses the same memory both with translations on (accessing as an 'effective address'), and with translations off (accessing as a 'real address'). This works in both guests and the hypervisor. For more details, see s5.7 of Book III of version 3 of the ISA, in particular the Storage Control Overview, s5.7.3, and s5.7.5 - noting that this KASAN implementation currently only supports Radix.] One approach is just to give up on inline instrumentation. This way all checks can be delayed until after everything set is up correctly, and the address-to-shadow calculations can be overridden. However, the features and speed boost provided by inline instrumentation are worth trying to do better. If _at compile time_ it is known how much contiguous physical memory a system has, the top 1/8th of the first block of physical memory can be set aside for the shadow. This is a big hammer and comes with 3 big consequences: - there's no nice way to handle physically discontiguous memory, so only the first physical memory block can be used. - kernels will simply fail to boot on machines with less memory than specified when compiling. - kernels running on machines with more memory than specified when compiling will simply ignore the extra memory. Implement and document KASAN this way. The current implementation is Radix only. Despite the limitations, it can still find bugs, e.g. http://patchwork.ozlabs.org/patch/1103775/ At the moment, this physical memory limit must be set _even for outline mode_. This may be changed in a later series - a different implementation could be added for outline mode that dynamically allocates shadow at a fixed offset. For example, see https://patchwork.ozlabs.org/patch/795211/ Suggested-by: Michael Ellerman Cc: Balbir Singh # ppc64 out-of-line radix version Cc: Christophe Leroy # ppc32 version Signed-off-by: Daniel Axtens --- Changes since v6: - rework kasan_late_init support, which also fixes book3e problem that snowpatch picked up (I think) - fix a checkpatch error that snowpatch picked up - don't needlessly move the include in kasan.h Changes since v5: - rebase on powerpc/merge, with Christophe's latest changes integrating kasan-vmalloc - documentation tweaks based on latest 32-bit changes Changes since v4: - fix some ppc32 build issues - support ptdump - clean up the header file. It turns out we don't need or use KASAN_SHADOW_SIZE, so just dump it, and make KASAN_SHADOW_END the thing that varies between 32 and 64 bit. As part of this, make sure KASAN_SHADOW_OFFSET is only configured for 32 bit - it is calculated in the Makefile for ppc64. - various cleanups Changes since v3: - Address further feedback from Christophe. - Drop changes to stack walking, it looks like the issue I observed is related to that particular stack, not stack-walking generally. Changes since v2: - Address feedback from Christophe around cleanups and docs. - Address feedback from Balbir: at this point I don't have a good solution for the issues you identify around the limitations of the inline implementation but I think that it's worth trying to get the stack instrumentation support. I'm happy to have an alternative and more flexible outline mode - I had envisoned this would be called 'lightweight' mode as it imposes fewer restrictions. I've linked to your implementation. I think it's best to add it in a follow-up series. - Made the default PHYS_MEM_SIZE_FOR_KASAN value 1024MB. I think most people have guests with at least that much memory in the Radix 64s case so it's a much saner default - it means that if you just turn on KASAN without reading the docs you're much more likely to have a bootable kernel, which you will never have if the value is set to zero! I'm happy to bikeshed the value if we want. Changes since v1: - Landed kasan vmalloc support upstream - Lots of feedback from Christophe. Changes since the rfc: - Boots real and virtual hardware, kvm works. - disabled reporting when we're checking the stack for exception frames. The behaviour isn't wrong, just incompatible with KASAN. - Documentation! - Dropped
