[PATCH] efivarfs: Implement exclusive access for {get,set}_variable

[PATCH] efivarfs: Implement exclusive access for {get,set}_variable

[Jeremy Kerr]

Currently, efivarfs does not enforce exclusion over the get_variable and
set_variable operations. Section 7.1 of UEFI requires us to only allow a
single processor to enter {get,set}_variable services at once.

This change acquires the efivars->lock over calls to these operations
from the efivarfs paths.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

---
 drivers/firmware/efivars.c |   68 +++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 5765664..a86eb55 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -679,35 +679,45 @@ static ssize_t efivarfs_file_write(struct file *file,
 		goto out;
 	}
 
+	/*
+	 * The lock here protects the get_variable call, the conditional
+	 * set_variable call, and removal of the variable from the efivars
+	 * list (in the case of an authenticated delete).
+	 */
+	spin_lock(&efivars->lock);
+
 	status = efivars->ops->set_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
 					    attributes, datasize,
 					    data);
 
-	switch (status) {
-	case EFI_SUCCESS:
-		break;
-	case EFI_INVALID_PARAMETER:
-		count = -EINVAL;
-		goto out;
-	case EFI_OUT_OF_RESOURCES:
-		count = -ENOSPC;
-		goto out;
-	case EFI_DEVICE_ERROR:
-		count = -EIO;
-		goto out;
-	case EFI_WRITE_PROTECTED:
-		count = -EROFS;
-		goto out;
-	case EFI_SECURITY_VIOLATION:
-		count = -EACCES;
-		goto out;
-	case EFI_NOT_FOUND:
-		count = -ENOENT;
-		goto out;
-	default:
-		count = -EINVAL;
-		goto out;
+	if (status != EFI_SUCCESS) {
+		spin_unlock(&efivars->lock);
+		kfree(data);
+
+		switch (status) {
+		case EFI_INVALID_PARAMETER:
+			count = -EINVAL;
+			break;
+		case EFI_OUT_OF_RESOURCES:
+			count = -ENOSPC;
+			break;
+		case EFI_DEVICE_ERROR:
+			count = -EIO;
+			break;
+		case EFI_WRITE_PROTECTED:
+			count = -EROFS;
+			break;
+		case EFI_SECURITY_VIOLATION:
+			count = -EACCES;
+			break;
+		case EFI_NOT_FOUND:
+			count = -ENOENT;
+			break;
+		default:
+			count = -EINVAL;
+		}
+		return count;
 	}
 
 	/*
@@ -723,12 +733,12 @@ static ssize_t efivarfs_file_write(struct file *file,
 					    NULL);
 
 	if (status == EFI_BUFFER_TOO_SMALL) {
+		spin_unlock(&efivars->lock);
 		mutex_lock(&inode->i_mutex);
 		i_size_write(inode, newdatasize + sizeof(attributes));
 		mutex_unlock(&inode->i_mutex);
 
 	} else if (status == EFI_NOT_FOUND) {
-		spin_lock(&efivars->lock);
 		list_del(&var->list);
 		spin_unlock(&efivars->lock);
 		efivar_unregister(var);
@@ -736,6 +746,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 		dput(file->f_dentry);
 
 	} else {
+		spin_unlock(&efivars->lock);
 		pr_warn("efivarfs: inconsistent EFI variable implementation? "
 				"status = %lx\n", status);
 	}
@@ -757,9 +768,11 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	void *data;
 	ssize_t size = 0;
 
+	spin_lock(&efivars->lock);
 	status = efivars->ops->get_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
 					    &attributes, &datasize, NULL);
+	spin_unlock(&efivars->lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return 0;
@@ -769,10 +782,13 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	if (!data)
 		return 0;
 
+	spin_lock(&efivars->lock);
 	status = efivars->ops->get_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
 					    &attributes, &datasize,
 					    (data + 4));
+	spin_unlock(&efivars->lock);
+
 	if (status != EFI_SUCCESS)
 		goto out_free;
 
@@ -993,11 +1009,13 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 		/* copied by the above to local storage in the dentry. */
 		kfree(name);
 
+		spin_lock(&efivars->lock);
 		efivars->ops->get_variable(entry->var.VariableName,
 					   &entry->var.VendorGuid,
 					   &entry->var.Attributes,
 					   &size,
 					   NULL);
+		spin_unlock(&efivars->lock);
 
 		mutex_lock(&inode->i_mutex);
 		inode->i_private = entry;

Re: [PATCH] efivarfs: Implement exclusive access for {get,set}_variable

[Matt Fleming]

On Thu, 2012-10-11 at 21:19 +0800, Jeremy Kerr wrote:
> Currently, efivarfs does not enforce exclusion over the get_variable and
> set_variable operations. Section 7.1 of UEFI requires us to only allow a
> single processor to enter {get,set}_variable services at once.
> 
> This change acquires the efivars->lock over calls to these operations
> from the efivarfs paths.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> ---
>  drivers/firmware/efivars.c |   68 +++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 25 deletions(-)

Thanks, applied to 'next'.

Re: [PATCH] efivarfs: Implement exclusive access for {get,set}_variable

[Greg KH]

On Thu, Oct 11, 2012 at 02:42:36PM +0100, Matt Fleming wrote:
> On Thu, 2012-10-11 at 21:19 +0800, Jeremy Kerr wrote:
> > Currently, efivarfs does not enforce exclusion over the get_variable and
> > set_variable operations. Section 7.1 of UEFI requires us to only allow a
> > single processor to enter {get,set}_variable services at once.
> > 
> > This change acquires the efivars->lock over calls to these operations
> > from the efivarfs paths.
> > 
> > Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> > 
> > ---
> >  drivers/firmware/efivars.c |   68 +++++++++++++++++++++++--------------
> >  1 file changed, 43 insertions(+), 25 deletions(-)
> 
> Thanks, applied to 'next'.

Should this be backported to the stable kernels?

thanks,

greg k-h

Re: [PATCH] efivarfs: Implement exclusive access for {get,set}_variable

[Jeremy Kerr]

Hi Greg,

> Should this be backported to the stable kernels?

No, the efivarfs code that this touches was only recently committed; it 
won't be in any of the stable series.

Cheers,


Jeremy

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] efivarfs: Implement exclusive access for {get,set}_variable

[Jeremy Kerr]

Hi Greg,

> Should this be backported to the stable kernels?

No, the efivarfs code that this touches was only recently committed; it 
won't be in any of the stable series.

Cheers,


Jeremy