[PATCH 0/4] efi: verify that variable services are supported

[PATCH 0/4] efi: verify that variable services are supported

[Johan Hovold]

This series adds a sanity check to make sure that the variable services
are actually available before registering the generic efivar ops.

This is used to address some potential races with custom efivars
implementation such as the Google SMI or upcoming Qualcomm SCM ones.

Specifically, efivarfs currently requires that the efivar ops have been
registered before module init time even though the Google driver can be
built as a module. Instead, the driver has so far relied on the fact
that the generic ops have been registered by efi core only to later be
overridden by the custom implementation (or Google doesn't use
efivarfs).

Instead, let's move the efivars sanity check to mount time to allow for
late registration of efivars.

Note that requiring that all efivars implementation to always be
built-in and registered before module init time could be an alternative,
but we'd still need to make sure that the custom implementation is then
not overridden by the default (broken) one. To avoid such init call
games, allowing late registration seems preferable.

This would however require any drivers that use efivars to probe defer
until it becomes available, which is also unfortunate, but possibly
still better than having generic kernels carry multiple built-in efivars
implementations.

Note that there are currently no such (efivars consumer) drivers in-tree
except for the efivars_pstore driver, which currently do rely on
efivarfs being available at module init time (and hence may fail to
initialise with the custom efivar implementations).

Johan


Johan Hovold (4):
  efi: efivars: add efivars printk prefix
  efivarfs: always register filesystem
  efi: verify that variable services are supported
  efi: efivars: prevent double registration

 drivers/firmware/efi/efi.c  | 22 ++++++++++++++++++++++
 drivers/firmware/efi/vars.c | 17 ++++++++++++++---
 fs/efivarfs/super.c         |  6 +++---
 3 files changed, 39 insertions(+), 6 deletions(-)

-- 
2.38.2

[PATCH 1/4] efi: efivars: add efivars printk prefix

[Johan Hovold]

Add an 'efivars: ' printk prefix to make the log entries stand out more,
for example:

	efivars: Registered efivars operations

While at it, change the sole remaining direct printk() call to pr_err().

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/firmware/efi/vars.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index aa5ba38f81ff..f34e7741e0c3 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -6,6 +6,8 @@
  * Copyright (C) 2004 Intel Corporation <matthew.e.tolentino@intel.com>
  */
 
+#define pr_fmt(fmt) "efivars: " fmt
+
 #include <linux/types.h>
 #include <linux/sizes.h>
 #include <linux/errno.h>
@@ -90,7 +92,7 @@ int efivars_unregister(struct efivars *efivars)
 		return -EINTR;
 
 	if (!__efivars) {
-		printk(KERN_ERR "efivars not registered\n");
+		pr_err("efivars not registered\n");
 		rv = -EINVAL;
 		goto out;
 	}
-- 
2.38.2

[PATCH 2/4] efivarfs: always register filesystem

[Johan Hovold]

The efivar ops are typically registered at subsys init time so that
they are available when efivarfs is registered at module init time.

Other efivars implementations, such as Google SMI, exists and can
currently be build as modules which means that efivar may not be
available when efivarfs is initialised.

Move the efivar availability check from module init to when the
filesystem is mounted to allow late registration of efivars.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 fs/efivarfs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index f72c529c8ec3..b67d431c861a 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -194,6 +194,9 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	struct dentry *root;
 	int err;
 
+	if (!efivar_is_available())
+		return -EOPNOTSUPP;
+
 	sb->s_maxbytes          = MAX_LFS_FILESIZE;
 	sb->s_blocksize         = PAGE_SIZE;
 	sb->s_blocksize_bits    = PAGE_SHIFT;
@@ -256,9 +259,6 @@ static struct file_system_type efivarfs_type = {
 
 static __init int efivarfs_init(void)
 {
-	if (!efivar_is_available())
-		return -ENODEV;
-
 	return register_filesystem(&efivarfs_type);
 }
 
-- 
2.38.2

Re: [PATCH 2/4] efivarfs: always register filesystem

[Ard Biesheuvel]

(cc Peter, Heinrich)

On Thu, 19 Jan 2023 at 17:45, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The efivar ops are typically registered at subsys init time so that
> they are available when efivarfs is registered at module init time.
>
> Other efivars implementations, such as Google SMI, exists and can
> currently be build as modules which means that efivar may not be
> available when efivarfs is initialised.
>
> Move the efivar availability check from module init to when the
> filesystem is mounted to allow late registration of efivars.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

I think this change is fine in principle, but I 'm not sure if there
is user space code that the distros are carrying that might get
confused by this: beforehand, efivarfs would not exist in
/proc/filesystems and now, it will but trying to mount it might fail.


> ---
>  fs/efivarfs/super.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index f72c529c8ec3..b67d431c861a 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -194,6 +194,9 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
>         struct dentry *root;
>         int err;
>
> +       if (!efivar_is_available())
> +               return -EOPNOTSUPP;
> +
>         sb->s_maxbytes          = MAX_LFS_FILESIZE;
>         sb->s_blocksize         = PAGE_SIZE;
>         sb->s_blocksize_bits    = PAGE_SHIFT;
> @@ -256,9 +259,6 @@ static struct file_system_type efivarfs_type = {
>
>  static __init int efivarfs_init(void)
>  {
> -       if (!efivar_is_available())
> -               return -ENODEV;
> -
>         return register_filesystem(&efivarfs_type);
>  }
>
> --
> 2.38.2
>

Re: [PATCH 2/4] efivarfs: always register filesystem

[Johan Hovold]

On Fri, Jan 20, 2023 at 10:23:18AM +0100, Ard Biesheuvel wrote:
> (cc Peter, Heinrich)
> 
> On Thu, 19 Jan 2023 at 17:45, Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > The efivar ops are typically registered at subsys init time so that
> > they are available when efivarfs is registered at module init time.
> >
> > Other efivars implementations, such as Google SMI, exists and can
> > currently be build as modules which means that efivar may not be
> > available when efivarfs is initialised.
> >
> > Move the efivar availability check from module init to when the
> > filesystem is mounted to allow late registration of efivars.
> >
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> I think this change is fine in principle, but I 'm not sure if there
> is user space code that the distros are carrying that might get
> confused by this: beforehand, efivarfs would not exist in
> /proc/filesystems and now, it will but trying to mount it might fail.

User space must already handle mount failing since commit 483028edacab
("efivars: respect EFI_UNSUPPORTED return from firmware") so that should
not be an issue.

Johan

Re: [PATCH 2/4] efivarfs: always register filesystem

[Ard Biesheuvel]

On Fri, 20 Jan 2023 at 17:04, Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Jan 20, 2023 at 10:23:18AM +0100, Ard Biesheuvel wrote:
> > (cc Peter, Heinrich)
> >
> > On Thu, 19 Jan 2023 at 17:45, Johan Hovold <johan+linaro@kernel.org> wrote:
> > >
> > > The efivar ops are typically registered at subsys init time so that
> > > they are available when efivarfs is registered at module init time.
> > >
> > > Other efivars implementations, such as Google SMI, exists and can
> > > currently be build as modules which means that efivar may not be
> > > available when efivarfs is initialised.
> > >
> > > Move the efivar availability check from module init to when the
> > > filesystem is mounted to allow late registration of efivars.
> > >
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >
> > I think this change is fine in principle, but I 'm not sure if there
> > is user space code that the distros are carrying that might get
> > confused by this: beforehand, efivarfs would not exist in
> > /proc/filesystems and now, it will but trying to mount it might fail.
>
> User space must already handle mount failing since commit 483028edacab
> ("efivars: respect EFI_UNSUPPORTED return from firmware") so that should
> not be an issue.
>

Fair enough

[PATCH 3/4] efi: verify that variable services are supported

[Johan Hovold]

Current Qualcomm UEFI firmware does not implement the variable services
but not all revisions clear the corresponding bits in the RT_PROP table
services mask and instead the corresponding calls return
EFI_UNSUPPORTED.

This leads to efi core registering the generic efivar ops even when the
variable services are not supported or when they are accessed through
some other interface (e.g. Google SMI or the upcoming Qualcomm SCM
implementation).

Instead of playing games with init call levels to make sure that the
custom implementations are registered after the generic one, make sure
that get_next_variable() is actually supported before registering the
generic ops.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/firmware/efi/efi.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index a7f1a32537f1..03ccc4e8d1fc 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -187,8 +187,27 @@ static const struct attribute_group efi_subsys_attr_group = {
 static struct efivars generic_efivars;
 static struct efivar_operations generic_ops;
 
+static bool generic_ops_supported(void)
+{
+	unsigned long name_size;
+	efi_status_t status;
+	efi_char16_t name;
+	efi_guid_t guid;
+
+	name_size = sizeof(name);
+
+	status = efi.get_next_variable(&name_size, &name, &guid);
+	if (status == EFI_UNSUPPORTED)
+		return false;
+
+	return true;
+}
+
 static int generic_ops_register(void)
 {
+	if (!generic_ops_supported())
+		return 0;
+
 	generic_ops.get_variable = efi.get_variable;
 	generic_ops.get_next_variable = efi.get_next_variable;
 	generic_ops.query_variable_store = efi_query_variable_store;
@@ -202,6 +221,9 @@ static int generic_ops_register(void)
 
 static void generic_ops_unregister(void)
 {
+	if (!generic_ops.get_variable)
+		return;
+
 	efivars_unregister(&generic_efivars);
 }
 
-- 
2.38.2

[PATCH 4/4] efi: efivars: prevent double registration

[Johan Hovold]

Add the missing sanity check to efivars_register() so that it is no
longer possible to override an already registered set of efivar ops
(without first deregistering them).

This can help debug initialisation ordering issues where drivers have so
far unknowingly been relying on overriding the generic ops.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/firmware/efi/vars.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index f34e7741e0c3..bd75b87f5fc1 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -62,18 +62,27 @@ EXPORT_SYMBOL_GPL(efivar_is_available);
 int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops)
 {
+	int rv;
+
 	if (down_interruptible(&efivars_lock))
 		return -EINTR;
 
+	if (__efivars) {
+		pr_warn("efivars already registered\n");
+		rv = -EBUSY;
+		goto out;
+	}
+
 	efivars->ops = ops;
 
 	__efivars = efivars;
 
 	pr_info("Registered efivars operations\n");
-
+	rv = 0;
+out:
 	up(&efivars_lock);
 
-	return 0;
+	return rv;
 }
 EXPORT_SYMBOL_GPL(efivars_register);
 
-- 
2.38.2

Re: [PATCH 0/4] efi: verify that variable services are supported

[Ard Biesheuvel]

On Thu, 19 Jan 2023 at 17:45, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> This series adds a sanity check to make sure that the variable services
> are actually available before registering the generic efivar ops.
>
> This is used to address some potential races with custom efivars
> implementation such as the Google SMI or upcoming Qualcomm SCM ones.
>
> Specifically, efivarfs currently requires that the efivar ops have been
> registered before module init time even though the Google driver can be
> built as a module. Instead, the driver has so far relied on the fact
> that the generic ops have been registered by efi core only to later be
> overridden by the custom implementation (or Google doesn't use
> efivarfs).
>
> Instead, let's move the efivars sanity check to mount time to allow for
> late registration of efivars.
>
> Note that requiring that all efivars implementation to always be
> built-in and registered before module init time could be an alternative,
> but we'd still need to make sure that the custom implementation is then
> not overridden by the default (broken) one. To avoid such init call
> games, allowing late registration seems preferable.
>
> This would however require any drivers that use efivars to probe defer
> until it becomes available, which is also unfortunate, but possibly
> still better than having generic kernels carry multiple built-in efivars
> implementations.
>
> Note that there are currently no such (efivars consumer) drivers in-tree
> except for the efivars_pstore driver, which currently do rely on
> efivarfs being available at module init time (and hence may fail to
> initialise with the custom efivar implementations).
>
> Johan
>
>
> Johan Hovold (4):
>   efi: efivars: add efivars printk prefix
>   efivarfs: always register filesystem
>   efi: verify that variable services are supported
>   efi: efivars: prevent double registration
>

Queued up in efi/next - thanks.

>  drivers/firmware/efi/efi.c  | 22 ++++++++++++++++++++++
>  drivers/firmware/efi/vars.c | 17 ++++++++++++++---
>  fs/efivarfs/super.c         |  6 +++---
>  3 files changed, 39 insertions(+), 6 deletions(-)
>
> --
> 2.38.2
>