| Message ID | 20230722123850.634544-1-alexghiti@rivosinc.com |
|---|---|
| Headers | show |
| Series | riscv: Introduce KASLR | expand |
Hey Alex, On Sat, Jul 22, 2023 at 02:38:45PM +0200, Alexandre Ghiti wrote: > The following KASLR implementation allows to randomize the kernel mapping: > > - virtually: we expect the bootloader to provide a seed in the device-tree > - physically: only implemented in the EFI stub, it relies on the firmware to > provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation > hence the patch 3 factorizes KASLR related functions for riscv to take > advantage. > > The new virtual kernel location is limited by the early page table that only > has one PUD and with the PMD alignment constraint, the kernel can only take > < 512 positions. I gave this all a go today, it seems to do what it it says on the tin, and crashing my kernel does dump out an offset etc. Tested-by: Conor Dooley <conor.dooley@microchip.com> I'll hopefully get some time later in the week to go through the code. Cheers, Conor.
On Mon, Jul 24, 2023 at 4:32 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > Hey Alex, > > On Sat, Jul 22, 2023 at 02:38:45PM +0200, Alexandre Ghiti wrote: > > The following KASLR implementation allows to randomize the kernel mapping: > > > > - virtually: we expect the bootloader to provide a seed in the device-tree > > - physically: only implemented in the EFI stub, it relies on the firmware to > > provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation > > hence the patch 3 factorizes KASLR related functions for riscv to take > > advantage. > > > > The new virtual kernel location is limited by the early page table that only > > has one PUD and with the PMD alignment constraint, the kernel can only take > > < 512 positions. > > I gave this all a go today, it seems to do what it it says on the tin, > and crashing my kernel does dump out an offset etc. > > Tested-by: Conor Dooley <conor.dooley@microchip.com> Great, thanks for testing! > > I'll hopefully get some time later in the week to go through the code. I will be on holiday in 3 weeks, you have some time, no worries :) Thanks again, Alex > > Cheers, > Conor. >
Hi, Alex: 在 2023/7/22 20:38, Alexandre Ghiti 写道: > The following KASLR implementation allows to randomize the kernel mapping: > > - virtually: we expect the bootloader to provide a seed in the device-tree > - physically: only implemented in the EFI stub, it relies on the firmware to > provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation > hence the patch 3 factorizes KASLR related functions for riscv to take > advantage. > > The new virtual kernel location is limited by the early page table that only > has one PUD and with the PMD alignment constraint, the kernel can only take > < 512 positions. > I have gone through the code and tested this series with RiscVVirt edk2. All seems good to me, you can add : Tested-by: Song Shuai <songshuaishuai@tinylab.org> And a few questions about patch 2 ("riscv: Dump out kernel offset information on panic"): 1. The dump_kernel_offset() function would output "Kernel Offset: 0x0 from 0xffffffff80000000" when booting with "nokaslr" option. How about disabling the registration of "kernel_offset_notifier" with "nokaslr" option? 2. Inspired by patch 2, I added the Crash KASLR support based on this series [1]. So is it necessary to keep this patch if we have Crash KASLR support? [1]: https://lore.kernel.org/linux-riscv/20230815104800.705753-1-songshuaishuai@tinylab.org/T/#u > base-commit-tag: v6.5-rc1 > > Changes in v6: > * Fix reintroduced build failures by compiling kaslr.c only for arm64 > and riscv, as suggested by Ard > > Changes in v5: > * Renamed efi-stub-kaslr.c into kaslr.c and fix commit log of patch 3, > as suggested by Ard > * Removed stubs since the kaslr functions were moved to their own file > (and then does not trigger any build failure for architectures that do > not call those functions since they are in their own compilation unit) > > Changes in v4: > * Fix efi_get_kimg macro that returned nothing > * Moved new kaslr functions into their own files to avoid zboot link > failures, as suggested by Ard > > Changes in v3: > * Rebase on top of 6.4-rc2 > * Make RANDOMIZE_BASE depend on 64bit > * Fix efi_icache_sync and efi_get_kimg_min_align which were undefined > in x86 (and certainly other archs) > * Add patch 4 to fix warning on rv32 > > Changes in v2: > * Rebase on top of 6.3-rc1 > * Add a riscv cache sync after memcpying the kernel > * Add kaslr_offset implementation for KCOV > * Add forward declaration to quiet LLVM > > Alexandre Ghiti (5): > riscv: Introduce virtual kernel mapping KASLR > riscv: Dump out kernel offset information on panic > arm64: libstub: Move KASLR handling functions to kaslr.c > libstub: Fix compilation warning for rv32 > riscv: libstub: Implement KASLR by using generic functions > > arch/arm64/include/asm/efi.h | 2 + > arch/riscv/Kconfig | 19 +++ > arch/riscv/include/asm/efi.h | 2 + > arch/riscv/include/asm/page.h | 3 + > arch/riscv/kernel/image-vars.h | 1 + > arch/riscv/kernel/pi/Makefile | 2 +- > arch/riscv/kernel/pi/cmdline_early.c | 13 ++ > arch/riscv/kernel/pi/fdt_early.c | 30 ++++ > arch/riscv/kernel/setup.c | 25 ++++ > arch/riscv/mm/init.c | 36 ++++- > drivers/firmware/efi/libstub/Makefile | 4 +- > drivers/firmware/efi/libstub/arm64-stub.c | 117 ++-------------- > drivers/firmware/efi/libstub/efistub.h | 8 ++ > drivers/firmware/efi/libstub/kaslr.c | 159 ++++++++++++++++++++++ > drivers/firmware/efi/libstub/riscv-stub.c | 33 ++--- > 15 files changed, 328 insertions(+), 126 deletions(-) > create mode 100644 arch/riscv/kernel/pi/fdt_early.c > create mode 100644 drivers/firmware/efi/libstub/kaslr.c >
Hi Song, On Tue, Aug 15, 2023 at 1:24 PM Song Shuai <songshuaishuai@tinylab.org> wrote: > > > Hi, Alex: > > 在 2023/7/22 20:38, Alexandre Ghiti 写道: > > The following KASLR implementation allows to randomize the kernel mapping: > > > > - virtually: we expect the bootloader to provide a seed in the device-tree > > - physically: only implemented in the EFI stub, it relies on the firmware to > > provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation > > hence the patch 3 factorizes KASLR related functions for riscv to take > > advantage. > > > > The new virtual kernel location is limited by the early page table that only > > has one PUD and with the PMD alignment constraint, the kernel can only take > > < 512 positions. > > > > I have gone through the code and tested this series with RiscVVirt edk2. > All seems good to me, you can add : > > Tested-by: Song Shuai <songshuaishuai@tinylab.org> > > And a few questions about patch 2 ("riscv: Dump out kernel offset > information on panic"): > > 1. The dump_kernel_offset() function would output "Kernel Offset: 0x0 > from 0xffffffff80000000" when booting with "nokaslr" option. > > How about disabling the registration of "kernel_offset_notifier" with > "nokaslr" option? I'd rather keep it as it shows the "nokaslr" flag was taken into account. > > 2. Inspired by patch 2, I added the Crash KASLR support based on this > series [1]. > So is it necessary to keep this patch if we have Crash KASLR support? I don't understand your question here > > > [1]: > https://lore.kernel.org/linux-riscv/20230815104800.705753-1-songshuaishuai@tinylab.org/T/#u > > > base-commit-tag: v6.5-rc1 > > > > Changes in v6: > > * Fix reintroduced build failures by compiling kaslr.c only for arm64 > > and riscv, as suggested by Ard > > > > Changes in v5: > > * Renamed efi-stub-kaslr.c into kaslr.c and fix commit log of patch 3, > > as suggested by Ard > > * Removed stubs since the kaslr functions were moved to their own file > > (and then does not trigger any build failure for architectures that do > > not call those functions since they are in their own compilation unit) > > > > Changes in v4: > > * Fix efi_get_kimg macro that returned nothing > > * Moved new kaslr functions into their own files to avoid zboot link > > failures, as suggested by Ard > > > > Changes in v3: > > * Rebase on top of 6.4-rc2 > > * Make RANDOMIZE_BASE depend on 64bit > > * Fix efi_icache_sync and efi_get_kimg_min_align which were undefined > > in x86 (and certainly other archs) > > * Add patch 4 to fix warning on rv32 > > > > Changes in v2: > > * Rebase on top of 6.3-rc1 > > * Add a riscv cache sync after memcpying the kernel > > * Add kaslr_offset implementation for KCOV > > * Add forward declaration to quiet LLVM > > > > Alexandre Ghiti (5): > > riscv: Introduce virtual kernel mapping KASLR > > riscv: Dump out kernel offset information on panic > > arm64: libstub: Move KASLR handling functions to kaslr.c > > libstub: Fix compilation warning for rv32 > > riscv: libstub: Implement KASLR by using generic functions > > > > arch/arm64/include/asm/efi.h | 2 + > > arch/riscv/Kconfig | 19 +++ > > arch/riscv/include/asm/efi.h | 2 + > > arch/riscv/include/asm/page.h | 3 + > > arch/riscv/kernel/image-vars.h | 1 + > > arch/riscv/kernel/pi/Makefile | 2 +- > > arch/riscv/kernel/pi/cmdline_early.c | 13 ++ > > arch/riscv/kernel/pi/fdt_early.c | 30 ++++ > > arch/riscv/kernel/setup.c | 25 ++++ > > arch/riscv/mm/init.c | 36 ++++- > > drivers/firmware/efi/libstub/Makefile | 4 +- > > drivers/firmware/efi/libstub/arm64-stub.c | 117 ++-------------- > > drivers/firmware/efi/libstub/efistub.h | 8 ++ > > drivers/firmware/efi/libstub/kaslr.c | 159 ++++++++++++++++++++++ > > drivers/firmware/efi/libstub/riscv-stub.c | 33 ++--- > > 15 files changed, 328 insertions(+), 126 deletions(-) > > create mode 100644 arch/riscv/kernel/pi/fdt_early.c > > create mode 100644 drivers/firmware/efi/libstub/kaslr.c > > > > -- > Thanks > Song Shuai Thanks for testing this and your suggestions! Alex
在 2023/8/17 21:10, Alexandre Ghiti 写道: > Hi Song, > > On Tue, Aug 15, 2023 at 1:24 PM Song Shuai <songshuaishuai@tinylab.org> wrote: >> >> >> Hi, Alex: >> >> 在 2023/7/22 20:38, Alexandre Ghiti 写道: >>> The following KASLR implementation allows to randomize the kernel mapping: >>> >>> - virtually: we expect the bootloader to provide a seed in the device-tree >>> - physically: only implemented in the EFI stub, it relies on the firmware to >>> provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation >>> hence the patch 3 factorizes KASLR related functions for riscv to take >>> advantage. >>> >>> The new virtual kernel location is limited by the early page table that only >>> has one PUD and with the PMD alignment constraint, the kernel can only take >>> < 512 positions. >>> >> >> I have gone through the code and tested this series with RiscVVirt edk2. >> All seems good to me, you can add : >> >> Tested-by: Song Shuai <songshuaishuai@tinylab.org> >> >> And a few questions about patch 2 ("riscv: Dump out kernel offset >> information on panic"): >> >> 1. The dump_kernel_offset() function would output "Kernel Offset: 0x0 >> from 0xffffffff80000000" when booting with "nokaslr" option. >> >> How about disabling the registration of "kernel_offset_notifier" with >> "nokaslr" option? > > I'd rather keep it as it shows the "nokaslr" flag was taken into account. > >> >> 2. Inspired by patch 2, I added the Crash KASLR support based on this >> series [1]. >> So is it necessary to keep this patch if we have Crash KASLR support? > > I don't understand your question here Crash can automatically calculate virt_offset by comparing the vmlinux and vmcore. If this patch is just intended to assist Crash in setting the "--kaslr offset," it might be deleted; if not just keep it. > >> >> >> [1]: >> https://lore.kernel.org/linux-riscv/20230815104800.705753-1-songshuaishuai@tinylab.org/T/#u >> >>> base-commit-tag: v6.5-rc1 >>> >>> Changes in v6: >>> * Fix reintroduced build failures by compiling kaslr.c only for arm64 >>> and riscv, as suggested by Ard >>> >>> Changes in v5: >>> * Renamed efi-stub-kaslr.c into kaslr.c and fix commit log of patch 3, >>> as suggested by Ard >>> * Removed stubs since the kaslr functions were moved to their own file >>> (and then does not trigger any build failure for architectures that do >>> not call those functions since they are in their own compilation unit) >>> >>> Changes in v4: >>> * Fix efi_get_kimg macro that returned nothing >>> * Moved new kaslr functions into their own files to avoid zboot link >>> failures, as suggested by Ard >>> >>> Changes in v3: >>> * Rebase on top of 6.4-rc2 >>> * Make RANDOMIZE_BASE depend on 64bit >>> * Fix efi_icache_sync and efi_get_kimg_min_align which were undefined >>> in x86 (and certainly other archs) >>> * Add patch 4 to fix warning on rv32 >>> >>> Changes in v2: >>> * Rebase on top of 6.3-rc1 >>> * Add a riscv cache sync after memcpying the kernel >>> * Add kaslr_offset implementation for KCOV >>> * Add forward declaration to quiet LLVM >>> >>> Alexandre Ghiti (5): >>> riscv: Introduce virtual kernel mapping KASLR >>> riscv: Dump out kernel offset information on panic >>> arm64: libstub: Move KASLR handling functions to kaslr.c >>> libstub: Fix compilation warning for rv32 >>> riscv: libstub: Implement KASLR by using generic functions >>> >>> arch/arm64/include/asm/efi.h | 2 + >>> arch/riscv/Kconfig | 19 +++ >>> arch/riscv/include/asm/efi.h | 2 + >>> arch/riscv/include/asm/page.h | 3 + >>> arch/riscv/kernel/image-vars.h | 1 + >>> arch/riscv/kernel/pi/Makefile | 2 +- >>> arch/riscv/kernel/pi/cmdline_early.c | 13 ++ >>> arch/riscv/kernel/pi/fdt_early.c | 30 ++++ >>> arch/riscv/kernel/setup.c | 25 ++++ >>> arch/riscv/mm/init.c | 36 ++++- >>> drivers/firmware/efi/libstub/Makefile | 4 +- >>> drivers/firmware/efi/libstub/arm64-stub.c | 117 ++-------------- >>> drivers/firmware/efi/libstub/efistub.h | 8 ++ >>> drivers/firmware/efi/libstub/kaslr.c | 159 ++++++++++++++++++++++ >>> drivers/firmware/efi/libstub/riscv-stub.c | 33 ++--- >>> 15 files changed, 328 insertions(+), 126 deletions(-) >>> create mode 100644 arch/riscv/kernel/pi/fdt_early.c >>> create mode 100644 drivers/firmware/efi/libstub/kaslr.c >>> >> >> -- >> Thanks >> Song Shuai > > Thanks for testing this and your suggestions! > > Alex >
Hi Alexandre, On Sat, Jul 22, 2023 at 5:39 AM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > The following KASLR implementation allows to randomize the kernel mapping: > > - virtually: we expect the bootloader to provide a seed in the device-tree > - physically: only implemented in the EFI stub, it relies on the firmware to > provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation > hence the patch 3 factorizes KASLR related functions for riscv to take > advantage. > > The new virtual kernel location is limited by the early page table that only > has one PUD and with the PMD alignment constraint, the kernel can only take > < 512 positions. > > base-commit-tag: v6.5-rc1 Thanks for continuing to work on this! I reviewed the patches and the code looks correct to me. I also applied the series on top of v6.5 and after patching qemu to provide a kaslr-seed, I confirmed that the virtual offset appears to be random and is printed out when I panic the machine: # echo PANIC > /sys/kernel/debug/provoke-crash/DIRECT [ 17.510012] lkdtm: Performing direct entry PANIC [ 17.510411] Kernel panic - not syncing: dumptest [...] [ 17.518693] Kernel Offset: 0x32c00000 from 0xffffffff80000000 For the series: Reviewed-by: Sami Tolvanen <samitolvanen@google.com> I didn't test the EFI bits, but the rest of the series: Tested-by: Sami Tolvanen <samitolvanen@google.com> Conor, in another reply you mentioned you're planning on reviewing the patches as well. Did you have any feedback or concerns? Sami
On Wed, Aug 30, 2023 at 02:30:31PM -0700, Sami Tolvanen wrote: > Conor, in another reply you mentioned you're planning on reviewing the > patches as well. Did you have any feedback or concerns? I didn't even look at it again. Been really short on time & I guess just deleted it from my queue when I noticed I'd tested it before.
On Wed, Aug 30, 2023 at 02:30:31PM -0700, Sami Tolvanen wrote: > Hi Alexandre, > > On Sat, Jul 22, 2023 at 5:39 AM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > > > The following KASLR implementation allows to randomize the kernel mapping: > > > > - virtually: we expect the bootloader to provide a seed in the device-tree > > - physically: only implemented in the EFI stub, it relies on the firmware to > > provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation > > hence the patch 3 factorizes KASLR related functions for riscv to take > > advantage. > > > > The new virtual kernel location is limited by the early page table that only > > has one PUD and with the PMD alignment constraint, the kernel can only take > > < 512 positions. > > > > base-commit-tag: v6.5-rc1 > > Thanks for continuing to work on this! > > I reviewed the patches and the code looks correct to me. I also > applied the series on top of v6.5 and after patching qemu to provide a > kaslr-seed, I confirmed that the virtual offset appears to be random > and is printed out when I panic the machine: > > # echo PANIC > /sys/kernel/debug/provoke-crash/DIRECT > [ 17.510012] lkdtm: Performing direct entry PANIC > [ 17.510411] Kernel panic - not syncing: dumptest > [...] > [ 17.518693] Kernel Offset: 0x32c00000 from 0xffffffff80000000 > > For the series: > Reviewed-by: Sami Tolvanen <samitolvanen@google.com> > > I didn't test the EFI bits, but the rest of the series: > Tested-by: Sami Tolvanen <samitolvanen@google.com> > > Conor, in another reply you mentioned you're planning on reviewing the > patches as well. Did you have any feedback or concerns? > > Sami > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv In addition to testing this patch in QEMU by patching like Sami did, I also booted this with a Debian kernel and tested it with EFI. I was able to use lkdtm as Sami did to force a panic and see the kernel offset changing in both scenarios. Tested-by: Charlie Jenkins <charlie@rivosinc.com> - Charlie
The following KASLR implementation allows to randomize the kernel mapping: - virtually: we expect the bootloader to provide a seed in the device-tree - physically: only implemented in the EFI stub, it relies on the firmware to provide a seed using EFI_RNG_PROTOCOL. arm64 has a similar implementation hence the patch 3 factorizes KASLR related functions for riscv to take advantage. The new virtual kernel location is limited by the early page table that only has one PUD and with the PMD alignment constraint, the kernel can only take < 512 positions. base-commit-tag: v6.5-rc1 Changes in v6: * Fix reintroduced build failures by compiling kaslr.c only for arm64 and riscv, as suggested by Ard Changes in v5: * Renamed efi-stub-kaslr.c into kaslr.c and fix commit log of patch 3, as suggested by Ard * Removed stubs since the kaslr functions were moved to their own file (and then does not trigger any build failure for architectures that do not call those functions since they are in their own compilation unit) Changes in v4: * Fix efi_get_kimg macro that returned nothing * Moved new kaslr functions into their own files to avoid zboot link failures, as suggested by Ard Changes in v3: * Rebase on top of 6.4-rc2 * Make RANDOMIZE_BASE depend on 64bit * Fix efi_icache_sync and efi_get_kimg_min_align which were undefined in x86 (and certainly other archs) * Add patch 4 to fix warning on rv32 Changes in v2: * Rebase on top of 6.3-rc1 * Add a riscv cache sync after memcpying the kernel * Add kaslr_offset implementation for KCOV * Add forward declaration to quiet LLVM Alexandre Ghiti (5): riscv: Introduce virtual kernel mapping KASLR riscv: Dump out kernel offset information on panic arm64: libstub: Move KASLR handling functions to kaslr.c libstub: Fix compilation warning for rv32 riscv: libstub: Implement KASLR by using generic functions arch/arm64/include/asm/efi.h | 2 + arch/riscv/Kconfig | 19 +++ arch/riscv/include/asm/efi.h | 2 + arch/riscv/include/asm/page.h | 3 + arch/riscv/kernel/image-vars.h | 1 + arch/riscv/kernel/pi/Makefile | 2 +- arch/riscv/kernel/pi/cmdline_early.c | 13 ++ arch/riscv/kernel/pi/fdt_early.c | 30 ++++ arch/riscv/kernel/setup.c | 25 ++++ arch/riscv/mm/init.c | 36 ++++- drivers/firmware/efi/libstub/Makefile | 4 +- drivers/firmware/efi/libstub/arm64-stub.c | 117 ++-------------- drivers/firmware/efi/libstub/efistub.h | 8 ++ drivers/firmware/efi/libstub/kaslr.c | 159 ++++++++++++++++++++++ drivers/firmware/efi/libstub/riscv-stub.c | 33 ++--- 15 files changed, 328 insertions(+), 126 deletions(-) create mode 100644 arch/riscv/kernel/pi/fdt_early.c create mode 100644 drivers/firmware/efi/libstub/kaslr.c