Re: git: f1e18f331923 - main - riscv: Exclude OpenSBI memory regions when booting with EFI

On 16 Apr 2025, at 15:21, Bojan Novković <bnovkov@FreeBSD.org> wrote:
> 
> The branch main has been updated by bnovkov:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=f1e18f331923041980149fef46cdb2736e61debb
> 
> commit f1e18f331923041980149fef46cdb2736e61debb
> Author:     Bojan Novković <bnovkov@FreeBSD.org>
> AuthorDate: 2025-04-15 16:28:05 +0000
> Commit:     Bojan Novković <bnovkov@FreeBSD.org>
> CommitDate: 2025-04-16 14:20:13 +0000
> 
>    riscv: Exclude OpenSBI memory regions when booting with EFI
> 
>    OpenSBI uses the first PMP entry to prevent buggy supervisor
>    software from overwriting the firmware [1]. However, this
>    region may not be properly marked as reserved in the EFI map, leading
>    to an access violation exception whenever the kernel
>    attempts to write to a page from that region.
> 
>    Fix this by preemptively excluding first EFI memory map entry
>    if it is marked as "BootServicesData".
> 
>    [1] https://github.com/riscv-non-isa/riscv-sbi-doc/pull/37
> 
>    Reported by: tuexen
>    Tested by: tuexen
>    Fixes: a2e2178402af
>    Reviewed by:    imp, jrtc27

No I didn’t, I left a comment saying I didn’t like the concept.

>    Differential Revision:  https://reviews.freebsd.org/D49839
> ---
> sys/riscv/riscv/machdep.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/sys/riscv/riscv/machdep.c b/sys/riscv/riscv/machdep.c
> index 516dbde5ffaa..f253bc9a853b 100644
> --- a/sys/riscv/riscv/machdep.c
> +++ b/sys/riscv/riscv/machdep.c
> @@ -541,6 +541,22 @@ fdt_physmem_exclude_region_cb(const struct mem_region *mr, void *arg __unused)
> }
> #endif
> 
> +static void
> +efi_exclude_sbi_pmp_cb(struct efi_md *p, void *argp)
> +{
> + bool *first = (bool *)argp;
> +
> + if (!*first)
> + return;
> +
> + *first = false;
> + if (p->md_type == EFI_MD_TYPE_BS_DATA) {
> + physmem_exclude_region(p->md_phys,
> +    min(p->md_pages * EFI_PAGE_SIZE, L2_SIZE),
> +    EXFLAG_NOALLOC);

Doesn’t this need EXFLAG_NODUMP like the FDT case?

Jess

> + }
> +}
> +
> void
> initriscv(struct riscv_bootparams *rvbp)
> {
> @@ -548,6 +564,7 @@ initriscv(struct riscv_bootparams *rvbp)
> struct pcpu *pcpup;
> vm_offset_t lastaddr;
> vm_size_t kernlen;
> + bool first;
> char *env;
> 
> TSRAW(&thread0, TS_ENTER, __func__, NULL);
> @@ -577,11 +594,22 @@ initriscv(struct riscv_bootparams *rvbp)
> if (efihdr != NULL) {
> efi_map_add_entries(efihdr);
> efi_map_exclude_entries(efihdr);
> +
> + /*
> + * OpenSBI uses the first PMP entry to prevent buggy supervisor
> + * software from overwriting the firmware. However, this
> + * region may not be properly marked as reserved, leading
> + * to an access violation exception whenever the kernel
> + * attempts to write to a page from that region.
> + *
> + * Fix this by excluding first EFI memory map entry
> + * if it is marked as "BootServicesData".
> + */
> + first = true;
> + efi_map_foreach_entry(efihdr, efi_exclude_sbi_pmp_cb, &first);
> }
> #ifdef FDT
> else {
> - bool first;
> -
> /* Exclude reserved memory specified by the device tree. */
> fdt_foreach_reserved_mem(fdt_physmem_exclude_region_cb, NULL);
>