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

OK, re-built a clean UEFI enabled aarch64 test VM and an x86 system and
compared:

aarch64:
[  312.161040] ZERO SIZED PTR: 0000000000000010
[  312.161252] doing copy_from_user..
[  312.161406] copy_from_user OK
[  312.161677] calling efi.get_next_variable..
[  312.162099] Unable to handle kernel NULL pointer dereference at
virtual address 00000010
[  312.162517] pgd = ffffffc03dfb8000
[  312.162759] [00000010] *pgd=000000007ba00003, *pmd=0000000000000000
[  312.163622] Internal error: Oops: 94000006 [#1] SMP

1. kmalloc(0) returns ZERO_SIZE_PTR on x86 and aarch64
2. is causes the null ptr deference issue on the efi.get_next_variable
call and not the copy_from_user

So, I guess the ARM EFI implementation of GetNextVariableName is
behaving differently.  Is is valid to pass a zero sized buffer into this
run time service, I suppose I was expecting a EFI_BUFFER_TOO_SMALL
return. However, we are also passing a non-NULL ptr to the name, which
will be interpreted as a valid buffer, but in fact it is not. So not
sure what to make of this.

a) We are passing an incorrect name ptr to the run time service, and it
is oopsing on that (which I guess is to be expected given we are passing
an invalid buffer over to it).
b) We are also passing a valid zero byte length, so perhaps the run time
service should return EFI_BUFFER_TOO_SMALL rather than proceeding.

Anyhow, I do believe we should pass a valid buffer over rather than
ZERO_SIZE_PTR.

Colin


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