ACK: [PATCH][v3] efi_runtime: ensure we don't allocate a zero byte buffer (LP: #1429890)]

[Alex Hung]

On 03/16/2015 09:27 PM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
> 
> When requesting a test on a zero sized buffer we must ensure that a
> buffer is allocated and is of zero size when passed to the EFI service.
> 
> If we kalloc zero bytes, then a ZERO_SIZE_PTR is returned, which has
> no meaning to the EFI service, so one strategy is to convert this to
> a NULL before passing it to the EFI service. However, this is handled
> by the service as an invalid parameter and returns with
> EFI_INVALID_PARAMETER rather than EFI_BUFFER_TOO_SMALL (the latter
> being what we are trying to actually exercise).
> 
> The best way forward is to allocate a buffer that is 1 wide char too
> long, and return back the buffer offset by 1 wide char (i.e skipping
> the leading 2 bytes). This allows us to always return a valid sized
> buffer that is passed to the EFI service, even with zero length requests.
> For zero sized buffers, if EFI writes into the zero sized buffer it will
> corrupt the member following the buffer.  I believe further checks should
> probably be added to also sanity check for these potential buffer overruns.
> 
> Across various platforms this arrangement trips the expected
> EFI_BUFFER_TOO_SMALL error return, as originally expected in the fwts userspace
> test case.
> 
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>  efi_runtime/efi_runtime.c | 44 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index ff31685..2708b4b 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -133,6 +133,15 @@ static inline size_t __ucs2_strsize(uint16_t  __user *str)
>  }
>  
>  /*
> + * Free a buffer allocated by copy_ucs2_from_user_len()
> + */
> +static inline void ucs2_kfree(uint16_t *buf)
> +{
> +	if (buf)
> +		kfree(buf - 1);
> +}
> +
> +/*
>   * Allocate a buffer and copy a ucs2 string from user space into it.
>   *
>   * We take an explicit number of bytes to use for the allocation and
> @@ -141,11 +150,22 @@ static inline size_t __ucs2_strsize(uint16_t  __user *str)
>   *
>   * If a non-zero value is returned, the caller MUST NOT access 'dst'.
>   *
> - * It is the caller's responsibility to free 'dst'.
> + * It is the caller's responsibility to free 'dst' using ucs2_kfree()
> + *
> + * We cater for zero sized len by always allocating a buffer that is 1
> + * uint16_t larger than the requested size and passing back the buffer
> + * offset by 1 uint16_t.  That way, the returned buffer size is the
> + * size that is requested and the buffer ptr is a valid pointer (and not
> + * NULL or ZERO_SIZE_PTR).  This allows EFI services to be passed a valid
> + * allocated buffer of zero length size and to see if the services handle
> + * this as an EFI_BUFFER_TOO_SMALL error.
> + * 
>   */
>  static inline int
>  copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len)
>  {
> +	uint16_t *buf;
> +
>  	if (!src) {
>  		*dst = NULL;
>  		return 0;
> @@ -154,12 +174,15 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len)
>  	if (!access_ok(VERIFY_READ, src, 1))
>  		return -EFAULT;
>  
> -	*dst = kmalloc(len, GFP_KERNEL);
> -	if (!*dst)
> +	buf = kmalloc(len + sizeof(uint16_t), GFP_KERNEL);
> +	if (!buf) {
> +		*dst = 0;
>  		return -ENOMEM;
> +	}
> +	*dst = buf + 1;
>  
>  	if (copy_from_user(*dst, src, len)) {
> -		kfree(*dst);
> +		kfree(buf);
>  		return -EFAULT;
>  	}
>  
> @@ -242,14 +265,13 @@ static long efi_runtime_get_variable(unsigned long arg)
>  
>  	data = kmalloc(datasize, GFP_KERNEL);
>  	if (!data) {
> -		kfree(name);
> +		ucs2_kfree(name);
>  		return -ENOMEM;
>  	}
>  
>  	prev_datasize = datasize;
>  	status = efi.get_variable(name, &vendor, &attr, &datasize, data);
> -
> -	kfree(name);
> +	ucs2_kfree(name);
>  
>  	if (status == EFI_SUCCESS && prev_datasize >= datasize)
>  		rv = copy_to_user(pgetvariable_local.Data, data, datasize);
> @@ -301,12 +323,12 @@ static long efi_runtime_set_variable(unsigned long arg)
>  
>  	data = kmalloc(psetvariable_local.DataSize, GFP_KERNEL);
>  	if (!data) {
> -		kfree(name);
> +		ucs2_kfree(name);
>  		return -ENOMEM;
>  	}
>  	if (copy_from_user(data, psetvariable_local.Data,
>  			   psetvariable_local.DataSize)) {
> -		kfree(data);
> +		ucs2_kfree(data);
>  		kfree(name);
>  		return -EFAULT;
>  	}
> @@ -315,7 +337,7 @@ static long efi_runtime_set_variable(unsigned long arg)
>  				  psetvariable_local.DataSize, data);
>  
>  	kfree(data);
> -	kfree(name);
> +	ucs2_kfree(name);
>  
>  	if (put_user(status, psetvariable_local.status))
>  		return -EFAULT;
> @@ -469,7 +491,7 @@ static long efi_runtime_get_nextvariablename(unsigned long arg)
>  	if (status == EFI_SUCCESS && prev_name_size >= name_size)
>  		rv = copy_ucs2_to_user_len(pgetnextvariablename_local.VariableName,
>  					   name, name_size);
> -	kfree(name);
> +	ucs2_kfree(name);
>  
>  	if (rv)
>  		return -EFAULT;
> 


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