ACK: [PATCH 3/5] efi_runtime: limit the amount of data copied to userspace

[Alex Hung]

On 02/06/2015 11:50 AM, Ricardo Neri wrote:
> The memory used to copy the variable name/data from userspace and back is
> supplied by the userspace. Thus, we must not write beyond the boundaries
> of the supplied memory. Otherwise, problems may arise (e.g., segmentation
> faults). However, the UEFI runtime service functions GetNextVariableName/
> GetVariable can change the size of the memory if, for instance, the
> buffer is too small. The firmware could also be defective.
> 
> Hence, only copy to the userspace the variable name/data if the UEFI
> functions return successfully. Also, in case there is a defect in the
> firmware, only copy to the userspace the variable name/data if the needed
> memory is less or equal to what the userspace provide.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon at linux.intel.com>
> ---
>  efi_runtime/efi_runtime.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index 1935aab..4ef1754 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -214,7 +214,7 @@ static long efi_runtime_get_variable(unsigned long arg)
>  {
>  	struct efi_getvariable __user *pgetvariable;
>  	struct efi_getvariable pgetvariable_local;
> -	unsigned long datasize;
> +	unsigned long datasize, prev_datasize;
>  	EFI_GUID vendor_guid;
>  	efi_guid_t vendor;
>  	efi_status_t status;
> @@ -246,11 +246,13 @@ static long efi_runtime_get_variable(unsigned long arg)
>  		return -ENOMEM;
>  	}
>  
> +	prev_datasize = datasize;
>  	status = efi.get_variable(name, &vendor, &attr, &datasize, data);
>  
>  	kfree(name);
>  
> -	rv = copy_to_user(pgetvariable_local.Data, data, datasize);
> +	if (status == EFI_SUCCESS && prev_datasize >= datasize)
> +		rv = copy_to_user(pgetvariable_local.Data, data, datasize);
>  	kfree(data);
>  
>  	if (rv)
> @@ -258,7 +260,7 @@ static long efi_runtime_get_variable(unsigned long arg)
>  
>  	if (put_user(status, pgetvariable_local.status))
>  		return -EFAULT;
> -	if (status == EFI_SUCCESS) {
> +	if (status == EFI_SUCCESS && prev_datasize >= datasize) {
>  		if (put_user(attr, pgetvariable_local.Attributes) ||
>  		    put_user(datasize, pgetvariable_local.DataSize))
>  			return -EFAULT;
> @@ -429,7 +431,7 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
>  {
>  	struct efi_getnextvariablename __user *pgetnextvariablename;
>  	struct efi_getnextvariablename pgetnextvariablename_local;
> -	unsigned long name_size;
> +	unsigned long name_size, prev_name_size;
>  	efi_status_t status;
>  	efi_guid_t vendor;
>  	EFI_GUID vendor_guid;
> @@ -459,10 +461,12 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
>  	if (rv)
>  		return rv;
>  
> +	prev_name_size = name_size;
>  	status = efi.get_next_variable(&name_size, name, &vendor);
>  
> -	rv = copy_ucs2_to_user_len(pgetnextvariablename_local.VariableName,
> -				   name, name_size);
> +	if (status == EFI_SUCCESS && prev_name_size >= name_size)
> +		rv = copy_ucs2_to_user_len(pgetnextvariablename_local.VariableName,
> +					   name, name_size);
>  	kfree(name);
>  
>  	if (rv)
> @@ -478,7 +482,7 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
>  	if (copy_to_user(pgetnextvariablename_local.VendorGuid,
>  			 &vendor_guid, sizeof(EFI_GUID)))
>  		return -EFAULT;
> -	if (status != EFI_SUCCESS)
> +	if (status != EFI_SUCCESS || name_size > prev_name_size)
>  		return -EINVAL;
>  	return 0;
>  }
> 


Acked-by: Alex Hung <alex.hung at canonical.com>