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

[Leif Lindholm]

> 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

I don't think this call ever reaches ARM-specific code:

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c#L2569

The runtime service simply passes on the pointer to "GetVariable",
which dereferences it to verify that it is not an empty string (which
has special meaning in this API).

I think dereferencing that pointer simply doesn't trigger an error on
x86.

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

The size is checked only before writing the next variable name into
the buffer.

The API specifies that the input should be a pointer to an empty
string or a string holding the name of the previous variable.

So, while one could add extra checks in edk2, that would be handling
more than the API requires, and guarantee nothing about non-edk2
implementations.
 
> Anyhow, I do believe we should pass a valid buffer over rather than
> ZERO_SIZE_PTR.

Yes please :)
 
/
    Leif