linux-kernel - Re: [PATCH V4 1/1] riscv/efi_stub: Add support for RISCV_EFI_BOOT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOnJCULUoedeARe0J=aduAVD35uHMe0MN9Pc_LDkavWZ-VtOkg@mail.gmail.com>
Date:   Thu, 19 May 2022 11:07:28 -0700
From:   Atish Patra <atishp@...shpatra.org>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Heinrich Schuchardt <heinrich.schuchardt@...onical.com>,
        Sunil V L <sunilvl@...tanamicro.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Atish Patra <atishp@...osinc.com>,
        Anup Patel <apatel@...tanamicro.com>,
        Jessica Clarke <jrtc27@...c27.com>,
        Abner Chang <abner.chang@....com>,
        linux-efi <linux-efi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Palmer Dabbelt <palmer@...osinc.com>
Subject: Re: [PATCH V4 1/1] riscv/efi_stub: Add support for RISCV_EFI_BOOT_PROTOCOL

On Thu, May 19, 2022 at 1:14 AM Ard Biesheuvel <ardb@...nel.org> wrote:
>
> On Thu, 19 May 2022 at 08:17, Heinrich Schuchardt
> <heinrich.schuchardt@...onical.com> wrote:
> >
> >
> >
> > On 5/18/22 23:03, Ard Biesheuvel wrote:
> > > On Wed, 18 May 2022 at 11:57, Sunil V L <sunilvl@...tanamicro.com> wrote:
> > >>
> > >> This patch adds the support for getting the boot hart ID in
> > >> Linux EFI stub using RISCV_EFI_BOOT_PROTOCOL. This protocol
> > >> is preferred method over existing DT based solution since it
> > >> works irrespective of DT or ACPI.
> > >>
> > >> The specification of the protocol is hosted at:
> > >> https://github.com/riscv-non-isa/riscv-uefi
> > >>
> > >> Signed-off-by: Sunil V L <sunilvl@...tanamicro.com>
> > >> Acked-by: Palmer Dabbelt <palmer@...osinc.com>
> > >> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@...onical.com>
> > >> ---
> > >>   drivers/firmware/efi/libstub/efistub.h    |  7 ++++++
> > >>   drivers/firmware/efi/libstub/riscv-stub.c | 29 +++++++++++++++++++----
> > >>   include/linux/efi.h                       |  1 +
> > >>   3 files changed, 32 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > >> index edb77b0621ea..aced62a0907e 100644
> > >> --- a/drivers/firmware/efi/libstub/efistub.h
> > >> +++ b/drivers/firmware/efi/libstub/efistub.h
> > >> @@ -720,6 +720,13 @@ union efi_tcg2_protocol {
> > >>          } mixed_mode;
> > >>   };
> > >>
> > >> +struct riscv_efi_boot_protocol {
> > >> +       u64 revision;
> > >> +
> > >> +       efi_status_t (__efiapi * get_boot_hartid)(struct riscv_efi_boot_protocol *this,
> > >> +                                                 size_t *boot_hartid);
> > >
> > > size_t is not a EFI type, and your spec uses UINTN here, which is
> > > equivalent to 'unsigned long'. However, jump_kernel_func() takes a
> > > unsigned int for the hartid. Please clean this up.
> >
> > unsigned long and size_t have the same number of bits. This seems to be
> > a question of taste where we should follow the maintainer.
> >
>
> We use unsigned long wherever the UEFI spec uses UINTN. This is not a
> matter of taste, really.
>
> > jump_kernel_func() assuming boot hart ID to be an unsigned int is not in
> > line with the RISC-V ISA which allows to use all xlen bits.
> >
> > >
> > >
> > >> +};
> > >> +
> > >>   typedef union efi_load_file_protocol efi_load_file_protocol_t;
> > >>   typedef union efi_load_file_protocol efi_load_file2_protocol_t;
> > >>
> > >> diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> > >> index 9c460843442f..012504f6f9a4 100644
> > >> --- a/drivers/firmware/efi/libstub/riscv-stub.c
> > >> +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > >> @@ -23,7 +23,7 @@
> > >>
> > >>   typedef void __noreturn (*jump_kernel_func)(unsigned int, unsigned long);
> > >>
> > >> -static u32 hartid;
> > >> +static size_t hartid;
> > >>
> > >
> > > and here
> > >
> > >>   static int get_boot_hartid_from_fdt(void)
> > >>   {
> > >> @@ -47,14 +47,33 @@ static int get_boot_hartid_from_fdt(void)
> > >>          return 0;
> > >>   }
> > >>
> > >> +static efi_status_t get_boot_hartid_from_efi(void)
> > >> +{
> > >> +       efi_guid_t boot_protocol_guid = RISCV_EFI_BOOT_PROTOCOL_GUID;
> > >> +       efi_status_t status;
> > >> +       struct riscv_efi_boot_protocol *boot_protocol;
> > >> +
> > >> +       status = efi_bs_call(locate_protocol, &boot_protocol_guid, NULL,
> > >> +                            (void **)&boot_protocol);
> > >> +       if (status == EFI_SUCCESS) {
> > >> +               status = efi_call_proto(boot_protocol,
> > >> +                                       get_boot_hartid, &hartid);
> >
> > A lot of the kernel code seems to be unfit to handle hart IDs exceeding
> > INT_MAX (e.g. sbi_cpu_is_stopped()). Until this is fixed we have to
> > treat hartid > INT_MAX as an error.
> >
>
> This is an issue in the core kernel code, not in the EFI stub. As you
> pointed out, the ISA implies UINTN / unsigned long here, and if the
> startup code cannot deal with that, it can be fixed separately.

It was kept as unsigned int because hartid > INT_MAX is very unlikely to happen.
But I agree that we should just follow the spec(allowing XLEN bits for
hartid) and
change "unsigned int" to "unsigned long" wherever hartid is concerned.

As the hartid is changed to unsigned long, get_boot_hartid_from_fdt
should be fixed as well.
Currently, it is using fdt32_to_cpu.



--
Regards,
Atish