Re: [PATCH RFC 2/5] nvmem: Add UEFI variable NVMEM provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmem: efivar: missing efivars_lock during EFI variable API calls
--

commit 95924ede9aa4d9717fd0738df67a068299d524f1
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>

nvmem: Add UEFI variable NVMEM provider

This commit adds a provider that reads a UEFI variable via the EFI runtime
GetVariable service and publishes its bytes as a read-only NVMEM device.

> diff --git a/drivers/nvmem/efivar.c b/drivers/nvmem/efivar.c
> new file mode 100644
> index 0000000000000..699b1d67aea44
> --- /dev/null
> +++ b/drivers/nvmem/efivar.c

[ ... ]

> @@ -68,26 +68,26 @@ static size_t efivar_nvmem_cells_extent(struct device *dev)
>  /*
>   * Find the vendor GUID of a variable given only its (UCS-2) name, by
>   * enumerating the variable store.  Used when no 'efivar-guid' is provided.
>   */
>  static int efivar_nvmem_find_guid(const efi_char16_t *want, efi_guid_t *out)
>  {
>  	unsigned long max = 512; /* in UCS-2 chars; variable names are short */
>  	efi_char16_t *name;
>  	efi_guid_t vendor = {};
>  	efi_status_t status;
>  	int ret = -ENODEV;
>  
>  	name = kcalloc(max, sizeof(*name), GFP_KERNEL);
>  	if (!name)
>  		return -ENOMEM;
>  	name[0] = 0;
>  
>  	for (;;) {
>  		unsigned long name_size = max * sizeof(*name);
>  
>  		status = efivar_get_next_variable(&name_size, name, &vendor);

[Severity: High]
Does this call to efivar_get_next_variable() require efivars_lock to be held
via efivar_lock()?

As documented in drivers/firmware/efi/vars.c, EFI variable APIs mandate that
efivars_lock is held by the caller. The lock protects the global __efivars
pointer and underlying firmware operations from concurrent state changes.

If the EFI variable backend (such as uefisecapp) is concurrently unregistered
via efivars_unregister(), __efivars is set to NULL under the lock. Without
serialization, the unprotected __efivars->ops->get_next_variable call inside
the EFI core will trigger a NULL pointer dereference.

>  		if (status == EFI_BUFFER_TOO_SMALL) {
>  			efi_char16_t *tmp;

[ ... ]

> @@ -180,24 +180,24 @@ static int efivar_nvmem_probe(struct platform_device *pdev)
>  		status = ret ? EFI_NOT_FOUND : EFI_SUCCESS;
>  	}
>  
>  	/* First attempt with a modest buffer; grow if the firmware asks. */
>  	size = 512;
>  	buf = kzalloc(size, GFP_KERNEL);
>  	if (!buf) {
>  		kfree(name16);
>  		return -ENOMEM;
>  	}
>  
>  	if (status == EFI_SUCCESS) {
>  		status = efivar_get_variable(name16, &guid, &attr, &size, buf);

[Severity: High]
Similar to efivar_get_next_variable() above, should this call to
efivar_get_variable() be protected by efivar_lock()?

Without the lock, an unprotected __efivars->ops->get_variable call inside
the EFI core can trigger a NULL pointer dereference if the backend is
concurrently unregistered.

>  		if (status == EFI_BUFFER_TOO_SMALL) {
>  			kfree(buf);
>  			buf = kzalloc(size, GFP_KERNEL);
>  			if (!buf) {
>  				kfree(name16);
>  				return -ENOMEM;
>  			}
>  			status = efivar_get_variable(name16, &guid, &attr,
>  						     &size, buf);

[Severity: High]
Does this second call to efivar_get_variable() also need to be protected
by efivar_lock()?

>  		}
>  	}
>  	kfree(name16);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-b4-nvmem-efivar-v1-0-187c85f65018@oss.qualcomm.com?part=2