On Thu, 12 Jan 2023 at 13:52, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> 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?
>
I'll respin it.
