On Wed, Jan 11, 2023 at 10:17 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Wed, 11 Jan 2023 at 21:23, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >
> > On Wed, Jan 11, 2023 at 2:27 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > >
> > > The ACPI PRM address space handler calls efi_call_virt_pointer() to
> > > execute PRM firmware code, but doing so is only permitted when the EFI
> > > runtime environment is available. Otherwise, such calls are guaranteed
> > > to result in a crash, and must therefore be avoided.
> > >
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > > Cc: Len Brown <lenb@xxxxxxxxxx>
> > > Cc: linux-acpi@xxxxxxxxxxxxxxx
> > > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > ---
> > > drivers/acpi/prmt.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> > > index 998101cf16e47145..74f924077866ae69 100644
> > > --- a/drivers/acpi/prmt.c
> > > +++ b/drivers/acpi/prmt.c
> > > @@ -236,6 +236,11 @@ static acpi_status acpi_platformrt_space_handler(u32 function,
> > > efi_status_t status;
> > > struct prm_context_buffer context;
> > >
> > > + if (!efi_enabled(EFI_RUNTIME_SERVICES)) {
> > > + pr_err("PRM: EFI runtime services unavailable\n");
> > > + return AE_NOT_IMPLEMENTED;
> > > + }
> > > +
> >
> > Does the check need to be made in the address space handler and if so, then why?
> >
>
> Yes. efi_enabled(EFI_RUNTIME_SERVICES) will transition from true to
> false if an exception occurs while executing the firmware code.
OK
> Unlike the EFI variable runtime services, which are quite uniform,
> this PRM code will be vendor specific, and so the likelihood that it
> is buggy and only tested with Windows is much higher, and so I would
> like us to be more cautious here.
OK
> > It looks like it would be better to prevent it from being installed if
> > EFI runtime services are not enabled in the first place, in
> > init_prmt().
> >
>
> We could add another check there as well, yes. And perhaps the one in
> the handler should we pr_warn_once() to prevent it from firing
> repeatedly.
Sounds good to me, so are you going to send an update of the patch?
Or how would you like to proceed otherwise?
> > > /*
> > > * The returned acpi_status will always be AE_OK. Error values will be
> > > * saved in the first byte of the PRM message buffer to be used by ASL.
> > > --
