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

[Ricardo Neri]

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


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

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