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

[Colin Ian King]

On 11/03/15 01:08, Neri, Ricardo wrote:
> 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.

Yes, looking at this again, it does do that.

> 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?

I'll check that out.

> 
>> 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'

+1, yep, I've taken that on board and will rethink this. Thanks

> 
> 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;
>>  
>