[4/4] efi_runtime: Do not pass user addresses to firmware

On Thu, Apr 03, 2014 at 03:23:23PM +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> Currently there's some inconsistency with how arguments are passed to
> the firmware from the efi_runtime driver. Some values have the standard
> get_user()/put_user() calls, others do not.
> 
> Passing userspace pointers directly to the firmware is a bug because
> those addresses may fault. And if they are going to fault we'd like to
> know about it in the kernel rather than at some later time when
> executing in firmware context.
> 
> Furthermore, beginning with v3.14 of the kernel the current tests
> actually cause the kernel to crash because firmware calls are now done
> with their own, entirely separate, page tables - no user addresses are
> mapped during execution of runtime services.
> 
> This change doesn't require predication on a particular kernel version
> because the efi_runtime should really have always done this copying
> to/from userspace for every argument of the runtime services.
> 
> This patch is heavily based on one from Borislav.
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  efi_runtime/efi_runtime.c | 239 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 215 insertions(+), 24 deletions(-)
> 
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index ffe107341470..d90d24eb151b 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -24,7 +24,7 @@
>  #include <linux/init.h>
>  #include <linux/proc_fs.h>
>  #include <linux/efi.h>
> -
> +#include <linux/slab.h>
>  #include <linux/uaccess.h>
>  
>  #include "efi_runtime.h"
> @@ -100,6 +100,110 @@ static void convert_to_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid)
>  		vendor_guid->Data4[i] = vendor->b[i+8];
>  }
>  
> +/*
> + * Count the bytes in 'str', including the terminating NULL.
> + *
> + * Note this function returns the number of *bytes*, not the number of
> + * ucs2 characters.
> + */
> +static inline size_t __strsize(uint16_t *str)
> +{
> +	uint16_t *s = str;
> +	size_t len;
> +
> +	if (!str)
> +		return 0;
> +
> +	/* Include terminating NULL */
> +	len = sizeof(uint16_t);
> +
> +	while (*s++ != 0)
> +		len += sizeof(uint16_t);
> +
> +	return len;
> +}
> +
> +/*
> + * Copy a ucs2 string from user space into a newly allocated kernel
> + * buffer.
> + *
> + * We take an explicit number of bytes to copy, and therefore do not
> + * make any assumptions about 'src' (such as it being a valid string).
> + */
> +static inline int
> +get_ucs2_len(uint16_t **dst, uint16_t __user *src, size_t len)

Just to show what I mean when we were talking on IRC: Since the caller
needs to free *dst, the comment over the function should state that, the
very least.

Then, we probably could call those functions something like
copy_uc2_to_user() and so on, to state that we're shuffling data to and
fro userspace and have the get/put_ucs2 names for allocating/freeing
buffers.

I'm not crazy about this version either (ontop of this patch) but let me
put it out there, just in case:

--

On Fri, 04 Apr, at 10:18:24AM, Borislav Petkov wrote:
> 
> Just to show what I mean when we were talking on IRC: Since the caller
> needs to free *dst, the comment over the function should state that, the
> very least.
 
Yeah, that's a good point as the calling is asymmetric wrt memory
allocation.

> Then, we probably could call those functions something like
> copy_uc2_to_user() and so on, to state that we're shuffling data to and
> fro userspace and have the get/put_ucs2 names for allocating/freeing
> buffers.

Another good idea, although I'm not keen on having a simple wrapper
around kfree() just to maintain symmetry. I do think the name changes
make sense though, but I'm happy to leave the kfree() stuff and simply
delete put_ucs2(), otherwise you get stuff like this,

In hindsight, get_* and put_* imply some kind of reference counting or
automatic alloc/free. I was really just trying to mirror put_user() and
get_user().

>  	data = kmalloc(datasize, GFP_KERNEL);
>  	if (copy_from_user(data, psetvariable->Data, datasize)) {
> -		kfree(name);
> +		put_ucs2(name);
>  		return -EFAULT;
>  	}
>  
>  	status = efi.set_variable(name, &vendor, attr, datasize, data);
>  
>  	kfree(data);
> -	kfree(name);
> +	put_ucs2(name);

where 'name' looks special in some way, but it's really not.

Colin, would you like me to send out a v2 with the improved comments,
deleted put_ucs2() (since it's unused) and improved naming convention?

On 04/04/14 10:46, Matt Fleming wrote:
> On Fri, 04 Apr, at 10:18:24AM, Borislav Petkov wrote:
>>
>> Just to show what I mean when we were talking on IRC: Since the caller
>> needs to free *dst, the comment over the function should state that, the
>> very least.
>  
> Yeah, that's a good point as the calling is asymmetric wrt memory
> allocation.
> 
>> Then, we probably could call those functions something like
>> copy_uc2_to_user() and so on, to state that we're shuffling data to and
>> fro userspace and have the get/put_ucs2 names for allocating/freeing
>> buffers.
> 
> Another good idea, although I'm not keen on having a simple wrapper
> around kfree() just to maintain symmetry. I do think the name changes
> make sense though, but I'm happy to leave the kfree() stuff and simply
> delete put_ucs2(), otherwise you get stuff like this,
> 
> In hindsight, get_* and put_* imply some kind of reference counting or
> automatic alloc/free. I was really just trying to mirror put_user() and
> get_user().
> 
>>  	data = kmalloc(datasize, GFP_KERNEL);
>>  	if (copy_from_user(data, psetvariable->Data, datasize)) {
>> -		kfree(name);
>> +		put_ucs2(name);
>>  		return -EFAULT;
>>  	}
>>  
>>  	status = efi.set_variable(name, &vendor, attr, datasize, data);
>>  
>>  	kfree(data);
>> -	kfree(name);
>> +	put_ucs2(name);
> 
> where 'name' looks special in some way, but it's really not.
> 
> Colin, would you like me to send out a v2 with the improved comments,
> deleted put_ucs2() (since it's unused) and improved naming convention?
> 
Matt, please do. Thanks

Colin