Re: [PATCH v6 0/5] riscv: Introduce KASLR

From: Charlie Jenkins <charlie@rivosinc.com>
To: Sami Tolvanen <samitolvanen@google.com>
Cc: Alexandre Ghiti <alexghiti@rivosinc.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Ard Biesheuvel <ardb@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-efi@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v6 0/5] riscv: Introduce KASLR
Date: Wed, 6 Sep 2023 16:27:56 -0700	[thread overview]
Message-ID: <ZPkK/MNGKZIW2mvc@ghost> (raw)
In-Reply-To: <CABCJKucj78p4TBtXYmMdJ=vYK69XC=XU06uB+_fyfZLRnC4h0A@mail.gmail.com>

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