diff mbox series

efi: efivars: Fix variable writes with unsupported query_variable_store()

Message ID 20221027135755.1732339-1-ardb@kernel.org
State Accepted
Commit f11a74b45d330ad1ab986852b099747161052526
Headers show
Series efi: efivars: Fix variable writes with unsupported query_variable_store() | expand

Commit Message

Ard Biesheuvel Oct. 27, 2022, 1:57 p.m. UTC
Commit 8a254d90a775 ("efi: efivars: Fix variable writes without
query_variable_store()") addressed an issue that was introduced during
the EFI variable store refactor, where alternative implementations of
the efivars layer that lacked query_variable_store() would no longer
work.

Unfortunately, there is another case to consider here, which was missed:
if the efivars layer is backed by the EFI runtime services as usual, but
the EFI implementation predates the introduction of QueryVariableInfo(),
we will return EFI_UNSUPPORTED, and this is no longer being dealt with
correctly.

So let's fix this, and while at it, clean up the code a bit, by merging
the check_var_size() routines as well as their callers.

Cc: <stable@vger.kernel.org> # v6.0
Fixes: bbc6d2c6ef22 ("efi: vars: Switch to new wrapper layer")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/vars.c | 68 +++++++++++--------------------------
 1 file changed, 20 insertions(+), 48 deletions(-)

Comments

Aditya Garg Oct. 28, 2022, 12:21 p.m. UTC | #1
Hi Ard

This patch you sent fixed my issue!

> On 27-Oct-2022, at 7:27 PM, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> Commit 8a254d90a775 ("efi: efivars: Fix variable writes without
> query_variable_store()") addressed an issue that was introduced during
> the EFI variable store refactor, where alternative implementations of
> the efivars layer that lacked query_variable_store() would no longer
> work.
> 
> Unfortunately, there is another case to consider here, which was missed:
> if the efivars layer is backed by the EFI runtime services as usual, but
> the EFI implementation predates the introduction of QueryVariableInfo(),
> we will return EFI_UNSUPPORTED, and this is no longer being dealt with
> correctly.
> 
> So let's fix this, and while at it, clean up the code a bit, by merging
> the check_var_size() routines as well as their callers.
> 

Thanks
Aditya
Ard Biesheuvel Oct. 28, 2022, 4:26 p.m. UTC | #2
On Fri, 28 Oct 2022 at 14:21, Aditya Garg <gargaditya08@live.com> wrote:
>
>
>
> Hi Ard
>
> This patch you sent fixed my issue!
>

Thanks for testing!


> > On 27-Oct-2022, at 7:27 PM, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Commit 8a254d90a775 ("efi: efivars: Fix variable writes without
> > query_variable_store()") addressed an issue that was introduced during
> > the EFI variable store refactor, where alternative implementations of
> > the efivars layer that lacked query_variable_store() would no longer
> > work.
> >
> > Unfortunately, there is another case to consider here, which was missed:
> > if the efivars layer is backed by the EFI runtime services as usual, but
> > the EFI implementation predates the introduction of QueryVariableInfo(),
> > we will return EFI_UNSUPPORTED, and this is no longer being dealt with
> > correctly.
> >
> > So let's fix this, and while at it, clean up the code a bit, by merging
> > the check_var_size() routines as well as their callers.
> >
>
> Thanks
> Aditya
diff mbox series

Patch

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 433b61587139..0ba9f18312f5 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -21,29 +21,22 @@  static struct efivars *__efivars;
 
 static DEFINE_SEMAPHORE(efivars_lock);
 
-static efi_status_t check_var_size(u32 attributes, unsigned long size)
-{
-	const struct efivar_operations *fops;
-
-	fops = __efivars->ops;
-
-	if (!fops->query_variable_store)
-		return (size <= SZ_64K) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
-
-	return fops->query_variable_store(attributes, size, false);
-}
-
-static
-efi_status_t check_var_size_nonblocking(u32 attributes, unsigned long size)
+static efi_status_t check_var_size(bool nonblocking, u32 attributes,
+				   unsigned long size)
 {
 	const struct efivar_operations *fops;
+	efi_status_t status;
 
 	fops = __efivars->ops;
 
 	if (!fops->query_variable_store)
+		status = EFI_UNSUPPORTED;
+	else
+		status = fops->query_variable_store(attributes, size,
+						    nonblocking);
+	if (status == EFI_UNSUPPORTED)
 		return (size <= SZ_64K) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
-
-	return fops->query_variable_store(attributes, size, true);
+	return status;
 }
 
 /**
@@ -195,26 +188,6 @@  efi_status_t efivar_get_next_variable(unsigned long *name_size,
 }
 EXPORT_SYMBOL_NS_GPL(efivar_get_next_variable, EFIVAR);
 
-/*
- * efivar_set_variable_blocking() - local helper function for set_variable
- *
- * Must be called with efivars_lock held.
- */
-static efi_status_t
-efivar_set_variable_blocking(efi_char16_t *name, efi_guid_t *vendor,
-			     u32 attr, unsigned long data_size, void *data)
-{
-	efi_status_t status;
-
-	if (data_size > 0) {
-		status = check_var_size(attr, data_size +
-					      ucs2_strsize(name, 1024));
-		if (status != EFI_SUCCESS)
-			return status;
-	}
-	return __efivars->ops->set_variable(name, vendor, attr, data_size, data);
-}
-
 /*
  * efivar_set_variable_locked() - set a variable identified by name/vendor
  *
@@ -228,23 +201,21 @@  efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
 	efi_set_variable_t *setvar;
 	efi_status_t status;
 
-	if (!nonblocking)
-		return efivar_set_variable_blocking(name, vendor, attr,
-						    data_size, data);
+	if (data_size > 0) {
+		status = check_var_size(nonblocking, attr,
+					data_size + ucs2_strsize(name, 1024));
+		if (status != EFI_SUCCESS)
+			return status;
+	}
 
 	/*
 	 * If no _nonblocking variant exists, the ordinary one
 	 * is assumed to be non-blocking.
 	 */
-	setvar = __efivars->ops->set_variable_nonblocking ?:
-		 __efivars->ops->set_variable;
+	setvar = __efivars->ops->set_variable_nonblocking;
+	if (!setvar || !nonblocking)
+		 setvar = __efivars->ops->set_variable;
 
-	if (data_size > 0) {
-		status = check_var_size_nonblocking(attr, data_size +
-							  ucs2_strsize(name, 1024));
-		if (status != EFI_SUCCESS)
-			return status;
-	}
 	return setvar(name, vendor, attr, data_size, data);
 }
 EXPORT_SYMBOL_NS_GPL(efivar_set_variable_locked, EFIVAR);
@@ -264,7 +235,8 @@  efi_status_t efivar_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 	if (efivar_lock())
 		return EFI_ABORTED;
 
-	status = efivar_set_variable_blocking(name, vendor, attr, data_size, data);
+	status = efivar_set_variable_locked(name, vendor, attr, data_size,
+					    data, false);
 	efivar_unlock();
 	return status;
 }