| Message ID | 20230111132734.1571990-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: PRM: Check whether EFI runtime is available | expand |
On Wed, Jan 11, 2023 at 2:27 PM Ard Biesheuvel <ardb@kernel.org> 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@vger.kernel.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Len Brown <lenb@kernel.org> > Cc: linux-acpi@vger.kernel.org > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > 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? 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(). > /* > * 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. > --
On Wed, 11 Jan 2023 at 21:23, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jan 11, 2023 at 2:27 PM Ard Biesheuvel <ardb@kernel.org> 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@vger.kernel.org> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Cc: Len Brown <lenb@kernel.org> > > Cc: linux-acpi@vger.kernel.org > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > 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. 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. > 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. > > /* > > * 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. > > --
On Wed, Jan 11, 2023 at 10:17 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Wed, 11 Jan 2023 at 21:23, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Jan 11, 2023 at 2:27 PM Ard Biesheuvel <ardb@kernel.org> 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@vger.kernel.org> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > > Cc: Len Brown <lenb@kernel.org> > > > Cc: linux-acpi@vger.kernel.org > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > 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. > > > --
On Thu, 12 Jan 2023 at 13:52, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jan 11, 2023 at 10:17 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Wed, 11 Jan 2023 at 21:23, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Wed, Jan 11, 2023 at 2:27 PM Ard Biesheuvel <ardb@kernel.org> 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@vger.kernel.org> > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > > > Cc: Len Brown <lenb@kernel.org> > > > > Cc: linux-acpi@vger.kernel.org > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > --- > > > > 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.
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; + } + /* * 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.
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@vger.kernel.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Len Brown <lenb@kernel.org> Cc: linux-acpi@vger.kernel.org Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/acpi/prmt.c | 5 +++++ 1 file changed, 5 insertions(+)