* [PATCH] ACPI: PRM: Check whether EFI runtime is available @ 2023-01-11 13:27 Ard Biesheuvel 2023-01-11 20:22 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2023-01-11 13:27 UTC (permalink / raw) To: linux-efi Cc: Ard Biesheuvel, stable, Rafael J. Wysocki, Len Brown, linux-acpi 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; + } + /* * 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. -- 2.39.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PRM: Check whether EFI runtime is available 2023-01-11 13:27 [PATCH] ACPI: PRM: Check whether EFI runtime is available Ard Biesheuvel @ 2023-01-11 20:22 ` Rafael J. Wysocki 2023-01-11 21:16 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2023-01-11 20:22 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-efi, stable, Rafael J. Wysocki, Len Brown, linux-acpi 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. > -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PRM: Check whether EFI runtime is available 2023-01-11 20:22 ` Rafael J. Wysocki @ 2023-01-11 21:16 ` Ard Biesheuvel 2023-01-12 12:51 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2023-01-11 21:16 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-efi, stable, Len Brown, linux-acpi 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. > > -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PRM: Check whether EFI runtime is available 2023-01-11 21:16 ` Ard Biesheuvel @ 2023-01-12 12:51 ` Rafael J. Wysocki 2023-01-12 12:52 ` Ard Biesheuvel 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2023-01-12 12:51 UTC (permalink / raw) To: Ard Biesheuvel Cc: Rafael J. Wysocki, linux-efi, stable, Len Brown, linux-acpi 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. > > > -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PRM: Check whether EFI runtime is available 2023-01-12 12:51 ` Rafael J. Wysocki @ 2023-01-12 12:52 ` Ard Biesheuvel 0 siblings, 0 replies; 5+ messages in thread From: Ard Biesheuvel @ 2023-01-12 12:52 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-efi, stable, Len Brown, linux-acpi 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-12 12:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-11 13:27 [PATCH] ACPI: PRM: Check whether EFI runtime is available Ard Biesheuvel 2023-01-11 20:22 ` Rafael J. Wysocki 2023-01-11 21:16 ` Ard Biesheuvel 2023-01-12 12:51 ` Rafael J. Wysocki 2023-01-12 12:52 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox