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

[Neri, Ricardo]

On Tue, 2015-03-10 at 17:25 +0000, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
> 
> BugLink: https://bugs.linaro.org/show_bug.cgi?id=1319
> 
> We are seeing kernel panics when the EFI userspace is exercising
> the efi runtime driver with a zero sized variable. The root cause
> is a zero byte kmalloc() which does not return zero and returns
> a buffer that we can't actually write into.

I think that what is happening here is that kmalloc returns
ZERO_SIZE_PTR, which is a valid return when doing kmalloc(0), AFAIK.
Also, where does it fail exactly? In the subsequent copy_from_user? I
wonder why it does not fail on x86? Could it be that ZERO_SIZE_PTR is
handled differently on copy_from_user to avoid panics when len=0?

> For zero bytes, allocate
> 1 byte buffer so we at least get a legitimate allocation of something.

Wouldn't this pass artificial data to the firmware? The user specified
zero size and yet we are creating a 1-byte buffer. The spirit of
de9c23134ac1b5ae5d5a666dd45e10820580dc4e "efi_runtime: get_nextvariable:
copy only the needed name bytes" was to not pass artificially-forgiving
buffers to the firmware to study its true behavior. What if dst+1 is
also '\0'? We will not know if the firmware checks the passed size of
the buffer or blindly checks for dst and dst+1 to be '\0'

Thanks and BR,
Ricardo	
> 
> Thanks for Naresh Bhat for bisecting the issue and testing.
> 
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> Reviewed/Tested By: Naresh Bhat <naresh.bhat at linaro.org>
> ---
>  efi_runtime/efi_runtime.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index ff31685..f2ccd57 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -154,7 +154,8 @@ 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);
> +	/* Ensure we don't kmalloc a zero byte buffer */
> +	*dst = kmalloc(len ? len : len + 1, GFP_KERNEL);
>  	if (!*dst)
>  		return -ENOMEM;
>