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

[Colin Ian King]

On 11/03/15 19:04, Ricardo Neri wrote:
> On Wed, 2015-03-11 at 11:32 +0000, Colin Ian King wrote:
>> 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.
> 
> Yes, I also think this is the expected result according to the spec:
> "The VariableNameSize is too small for the result."
>> 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.
> 
> I think that ZERO_SIZE_PTR vs NULL are Linux concepts. Thus, maybe the
> implementation of efi.get_next_variable should translate ZERO_SIZE_PTR
> into NULL before passing to the firmware?

Indeed, I agree ZERO_SIZE_PTR is a Linux concept, so we should not use
it and the kernel needs to translate it to NULL before passing to the
firmware.  UEFI should check for a NULL and return EFI_INVALID_PARAMETER
if it's doing the right thing.

>>
>> 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).
> 
> But why efi.get_next_variable needs to dereference the pointer? Page
> translation maybe?
> 
>> 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.
> 
> This should be the behavior if the runtime service has a chance to run.
> From your results, it seems that Linux is trying to dereference the
> non-NULL invalid buffer before even passing it to the service.

That does indeed seem so, but why I can't fathom why that occurs on
aarch64 at the present moment.

>>
>> Anyhow, I do believe we should pass a valid buffer over rather than
>> ZERO_SIZE_PTR.
> 
> Wouldn't this hide the very problem we are discussing here? For instance
> in drivers/firmware/google/gsmi.c we see the following:
> 
> static efi_status_t gsmi_get_next_variable(unsigned long *name_size,
> 					   efi_char16_t *name,
> 					   efi_guid_t *vendor)
> {
> 	...
> 	/* Let's make sure the thing is at least null-terminated */
> 	if (ucs2_strnlen(name, GSMI_BUF_SIZE / 2) == GSMI_BUF_SIZE / 2)
> 		return EFI_INVALID_PARAMETER;
> 	...
> }
> 
> The code blindly dereferences name without even checking if it is valid!

Yes, that is a particularly good example of a poor implementation. I
think we need to add more tests to fwts to explore invalid run time
calls and sanity check that the kernel and the firmware does the right
thing.

> 
> 
> In order for FWTS to run correctly, perhaps it would be good to put a
> note saying that this is the approach while the situation is resolved.

To clarify, which approach are you referring to?

1) ZERO_SIZE_PTR being replaced to a NULL or
2) allocating a 1 byte buffer and passing that over to stop the kernel
breaking.

Thanks

Colin

>>
> 
> Thanks and BR,
> Ricardo
>> 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;
>>>>  
>>>
>>
>>
> 
>