ACK: [PATCH 1/1] UBUNTU: SAUCE: efivarfs: Implement exclusive access for {get, set}_variable

[Colin Ian King]

On 18/10/12 16:41, Andy Whitcroft wrote:
> From: Jeremy Kerr <jeremy.kerr at canonical.com>
>
> BugLink: http://bugs.launchpad.net/bugs/1063061
>
> 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 at canonical.com>
> Signed-off-by: Matt Fleming <matt.fleming at intel.com>
> Signed-off-by: Andy Whitcroft <apw at 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 904d9f3..358fe2f 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -690,35 +690,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;
>   	}
>
>   	/*
> @@ -734,12 +744,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);
> @@ -747,6 +757,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);
>   	}
> @@ -768,9 +779,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;
> @@ -780,10 +793,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;
>
> @@ -1004,11 +1020,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;
>
Acked-by: Colin Ian King <colin.king at canonical.com>