WTF: patch "[PATCH] efi: Make it possible to disable efivar_ssdt entirely" was seriously submitted to be applied to the 5.7-stable tree?

WTF: patch "[PATCH] efi: Make it possible to disable efivar_ssdt entirely" was seriously submitted to be applied to the 5.7-stable tree?

[gregkh]

The patch below was submitted to be applied to the 5.7-stable tree.

I fail to see how this patch meets the stable kernel rules as found at
Documentation/process/stable-kernel-rules.rst.

I could be totally wrong, and if so, please respond to 
<stable@vger.kernel.org> and let me know why this patch should be
applied.  Otherwise, it is now dropped from my patch queues, never to be
seen again.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 435d1a471598752446a72ad1201b3c980526d869 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Mon, 15 Jun 2020 16:24:08 -0400
Subject: [PATCH] efi: Make it possible to disable efivar_ssdt entirely

In most cases, such as CONFIG_ACPI_CUSTOM_DSDT and
CONFIG_ACPI_TABLE_UPGRADE, boot-time modifications to firmware tables
are tied to specific Kconfig options.  Currently this is not the case
for modifying the ACPI SSDT via the efivar_ssdt kernel command line
option and associated EFI variable.

This patch adds CONFIG_EFI_CUSTOM_SSDT_OVERLAYS, which defaults
disabled, in order to allow enabling or disabling that feature during
the build.

Cc: <stable@vger.kernel.org>
Signed-off-by: Peter Jones <pjones@redhat.com>
Link: https://lore.kernel.org/r/20200615202408.2242614-1-pjones@redhat.com
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e6fc022bc87e..3939699e62fe 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -278,3 +278,14 @@ config EFI_EARLYCON
 	depends on SERIAL_EARLYCON && !ARM && !IA64
 	select FONT_SUPPORT
 	select ARCH_USE_MEMREMAP_PROT
+
+config EFI_CUSTOM_SSDT_OVERLAYS
+	bool "Load custom ACPI SSDT overlay from an EFI variable"
+	depends on EFI_VARS && ACPI
+	default ACPI_TABLE_UPGRADE
+	help
+	  Allow loading of an ACPI SSDT overlay from an EFI variable specified
+	  by a kernel command line option.
+
+	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
+	  information.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index edc5d36caf54..5114cae4ec97 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -189,7 +189,7 @@ static void generic_ops_unregister(void)
 	efivars_unregister(&generic_efivars);
 }
 
-#if IS_ENABLED(CONFIG_ACPI)
+#ifdef CONFIG_EFI_CUSTOM_SSDT_OVERLAYS
 #define EFIVAR_SSDT_NAME_MAX	16
 static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
 static int __init efivar_ssdt_setup(char *str)

Re: WTF: patch "[PATCH] efi: Make it possible to disable efivar_ssdt entirely" was seriously submitted to be applied to the 5.7-stable tree?

[Ard Biesheuvel]

On Mon, 29 Jun 2020 at 11:32, <gregkh@linuxfoundation.org> wrote:
>
> The patch below was submitted to be applied to the 5.7-stable tree.
>
> I fail to see how this patch meets the stable kernel rules as found at
> Documentation/process/stable-kernel-rules.rst.
>
> I could be totally wrong, and if so, please respond to
> <stable@vger.kernel.org> and let me know why this patch should be
> applied.  Otherwise, it is now dropped from my patch queues, never to be
> seen again.
>

Without this patch, there is no way to disable sideloading of SSDTs
via EFI variables, which is a security hole. The fact that this is not
governed by the existing ACPI_TABLE_UPGRADE Kconfig option was an
oversight, and so distros currently have this functionality enabled
inadvertently (although most of them have the lockdown check
incorporated as well)

SSDTs can manipulate any memory (even kernel memory that has been
mapped read-only) by using SystemMemory OpRegions in _INI AML methods,
and setting an EFI variable once will make this persist across
reboots.




> ------------------ original commit in Linus's tree ------------------
>
> From 435d1a471598752446a72ad1201b3c980526d869 Mon Sep 17 00:00:00 2001
> From: Peter Jones <pjones@redhat.com>
> Date: Mon, 15 Jun 2020 16:24:08 -0400
> Subject: [PATCH] efi: Make it possible to disable efivar_ssdt entirely
>
> In most cases, such as CONFIG_ACPI_CUSTOM_DSDT and
> CONFIG_ACPI_TABLE_UPGRADE, boot-time modifications to firmware tables
> are tied to specific Kconfig options.  Currently this is not the case
> for modifying the ACPI SSDT via the efivar_ssdt kernel command line
> option and associated EFI variable.
>
> This patch adds CONFIG_EFI_CUSTOM_SSDT_OVERLAYS, which defaults
> disabled, in order to allow enabling or disabling that feature during
> the build.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Link: https://lore.kernel.org/r/20200615202408.2242614-1-pjones@redhat.com
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index e6fc022bc87e..3939699e62fe 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -278,3 +278,14 @@ config EFI_EARLYCON
>         depends on SERIAL_EARLYCON && !ARM && !IA64
>         select FONT_SUPPORT
>         select ARCH_USE_MEMREMAP_PROT
> +
> +config EFI_CUSTOM_SSDT_OVERLAYS
> +       bool "Load custom ACPI SSDT overlay from an EFI variable"
> +       depends on EFI_VARS && ACPI
> +       default ACPI_TABLE_UPGRADE
> +       help
> +         Allow loading of an ACPI SSDT overlay from an EFI variable specified
> +         by a kernel command line option.
> +
> +         See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
> +         information.
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index edc5d36caf54..5114cae4ec97 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -189,7 +189,7 @@ static void generic_ops_unregister(void)
>         efivars_unregister(&generic_efivars);
>  }
>
> -#if IS_ENABLED(CONFIG_ACPI)
> +#ifdef CONFIG_EFI_CUSTOM_SSDT_OVERLAYS
>  #define EFIVAR_SSDT_NAME_MAX   16
>  static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
>  static int __init efivar_ssdt_setup(char *str)
>

Re: WTF: patch "[PATCH] efi: Make it possible to disable efivar_ssdt entirely" was seriously submitted to be applied to the 5.7-stable tree?

[Greg Kroah-Hartman]

On Mon, Jun 29, 2020 at 05:18:08PM +0200, Ard Biesheuvel wrote:
> On Mon, 29 Jun 2020 at 11:32, <gregkh@linuxfoundation.org> wrote:
> >
> > The patch below was submitted to be applied to the 5.7-stable tree.
> >
> > I fail to see how this patch meets the stable kernel rules as found at
> > Documentation/process/stable-kernel-rules.rst.
> >
> > I could be totally wrong, and if so, please respond to
> > <stable@vger.kernel.org> and let me know why this patch should be
> > applied.  Otherwise, it is now dropped from my patch queues, never to be
> > seen again.
> >
> 
> Without this patch, there is no way to disable sideloading of SSDTs
> via EFI variables, which is a security hole. The fact that this is not
> governed by the existing ACPI_TABLE_UPGRADE Kconfig option was an
> oversight, and so distros currently have this functionality enabled
> inadvertently (although most of them have the lockdown check
> incorporated as well)
> 
> SSDTs can manipulate any memory (even kernel memory that has been
> mapped read-only) by using SystemMemory OpRegions in _INI AML methods,
> and setting an EFI variable once will make this persist across
> reboots.

All of this was not in the description of the patch at all, how were we
supposed to know this?

And this really looks like a new feature now that you are supporting
something that we previously could not do.  To know that this is a "fix"
is not obvious :(

I'll go queue it up, but how far back should it go?

thanks,

greg k-h

Re: WTF: patch "[PATCH] efi: Make it possible to disable efivar_ssdt entirely" was seriously submitted to be applied to the 5.7-stable tree?

[Ard Biesheuvel]

On Mon, 29 Jun 2020 at 17:48, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 29, 2020 at 05:18:08PM +0200, Ard Biesheuvel wrote:
> > On Mon, 29 Jun 2020 at 11:32, <gregkh@linuxfoundation.org> wrote:
> > >
> > > The patch below was submitted to be applied to the 5.7-stable tree.
> > >
> > > I fail to see how this patch meets the stable kernel rules as found at
> > > Documentation/process/stable-kernel-rules.rst.
> > >
> > > I could be totally wrong, and if so, please respond to
> > > <stable@vger.kernel.org> and let me know why this patch should be
> > > applied.  Otherwise, it is now dropped from my patch queues, never to be
> > > seen again.
> > >
> >
> > Without this patch, there is no way to disable sideloading of SSDTs
> > via EFI variables, which is a security hole. The fact that this is not
> > governed by the existing ACPI_TABLE_UPGRADE Kconfig option was an
> > oversight, and so distros currently have this functionality enabled
> > inadvertently (although most of them have the lockdown check
> > incorporated as well)
> >
> > SSDTs can manipulate any memory (even kernel memory that has been
> > mapped read-only) by using SystemMemory OpRegions in _INI AML methods,
> > and setting an EFI variable once will make this persist across
> > reboots.
>
> All of this was not in the description of the patch at all, how were we
> supposed to know this?
>

Good point. This patch was the result of same off-list discussion, so
it was obvious to those involved but not for anyone else.

> And this really looks like a new feature now that you are supporting
> something that we previously could not do.  To know that this is a "fix"
> is not obvious :(
>
> I'll go queue it up, but how far back should it go?
>

The feature was added in v4.8, so as close as we can get to that please.

Thanks,
Ard.

Re: WTF: patch "[PATCH] efi: Make it possible to disable efivar_ssdt entirely" was seriously submitted to be applied to the 5.7-stable tree?

[Greg Kroah-Hartman]

On Mon, Jun 29, 2020 at 08:30:21PM +0200, Ard Biesheuvel wrote:
> On Mon, 29 Jun 2020 at 17:48, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jun 29, 2020 at 05:18:08PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 29 Jun 2020 at 11:32, <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > The patch below was submitted to be applied to the 5.7-stable tree.
> > > >
> > > > I fail to see how this patch meets the stable kernel rules as found at
> > > > Documentation/process/stable-kernel-rules.rst.
> > > >
> > > > I could be totally wrong, and if so, please respond to
> > > > <stable@vger.kernel.org> and let me know why this patch should be
> > > > applied.  Otherwise, it is now dropped from my patch queues, never to be
> > > > seen again.
> > > >
> > >
> > > Without this patch, there is no way to disable sideloading of SSDTs
> > > via EFI variables, which is a security hole. The fact that this is not
> > > governed by the existing ACPI_TABLE_UPGRADE Kconfig option was an
> > > oversight, and so distros currently have this functionality enabled
> > > inadvertently (although most of them have the lockdown check
> > > incorporated as well)
> > >
> > > SSDTs can manipulate any memory (even kernel memory that has been
> > > mapped read-only) by using SystemMemory OpRegions in _INI AML methods,
> > > and setting an EFI variable once will make this persist across
> > > reboots.
> >
> > All of this was not in the description of the patch at all, how were we
> > supposed to know this?
> >
> 
> Good point. This patch was the result of same off-list discussion, so
> it was obvious to those involved but not for anyone else.
> 
> > And this really looks like a new feature now that you are supporting
> > something that we previously could not do.  To know that this is a "fix"
> > is not obvious :(
> >
> > I'll go queue it up, but how far back should it go?
> >
> 
> The feature was added in v4.8, so as close as we can get to that please.

Ok, got it applied back to 4.9 now, thanks.

greg k-h

Re: WTF: patch "[PATCH] efi: Make it possible to disable efivar_ssdt entirely" was seriously submitted to be applied to the 5.7-stable tree?

[Ard Biesheuvel]

On Tue, 7 Jul 2020 at 17:10, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 29, 2020 at 08:30:21PM +0200, Ard Biesheuvel wrote:
> > On Mon, 29 Jun 2020 at 17:48, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 05:18:08PM +0200, Ard Biesheuvel wrote:
> > > > On Mon, 29 Jun 2020 at 11:32, <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > The patch below was submitted to be applied to the 5.7-stable tree.
> > > > >
> > > > > I fail to see how this patch meets the stable kernel rules as found at
> > > > > Documentation/process/stable-kernel-rules.rst.
> > > > >
> > > > > I could be totally wrong, and if so, please respond to
> > > > > <stable@vger.kernel.org> and let me know why this patch should be
> > > > > applied.  Otherwise, it is now dropped from my patch queues, never to be
> > > > > seen again.
> > > > >
> > > >
> > > > Without this patch, there is no way to disable sideloading of SSDTs
> > > > via EFI variables, which is a security hole. The fact that this is not
> > > > governed by the existing ACPI_TABLE_UPGRADE Kconfig option was an
> > > > oversight, and so distros currently have this functionality enabled
> > > > inadvertently (although most of them have the lockdown check
> > > > incorporated as well)
> > > >
> > > > SSDTs can manipulate any memory (even kernel memory that has been
> > > > mapped read-only) by using SystemMemory OpRegions in _INI AML methods,
> > > > and setting an EFI variable once will make this persist across
> > > > reboots.
> > >
> > > All of this was not in the description of the patch at all, how were we
> > > supposed to know this?
> > >
> >
> > Good point. This patch was the result of same off-list discussion, so
> > it was obvious to those involved but not for anyone else.
> >
> > > And this really looks like a new feature now that you are supporting
> > > something that we previously could not do.  To know that this is a "fix"
> > > is not obvious :(
> > >
> > > I'll go queue it up, but how far back should it go?
> > >
> >
> > The feature was added in v4.8, so as close as we can get to that please.
>
> Ok, got it applied back to 4.9 now, thanks.
>

Thanks Greg.