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

[Ricardo Neri]

On Wed, 2015-03-11 at 20:38 +0000, Leif Lindholm 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
> 
> 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.

>From the results of Colin, it appears that the panic happens before
calling the runtime service, still in the kernel side somewhere the
ZERO_SIZE_PTR seems to be dereferenced.
> 
> > 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.

But still, I think edk2 should defensively check that the inputs are
valid. And it does it as per your link. The difficulty is that edk2
cannot (and should not) tell the difference between ZERO_SIZE_PTR and
NULL. Thus, any code passing the call to the firmware should do that
translation.
> 
> 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.

Then that implementation should check the validity of the inputs.
>  
> > Anyhow, I do believe we should pass a valid buffer over rather than
> > ZERO_SIZE_PTR.
> 
> Yes please :)

The culprit situation that I'd like to expose is not so much on the
validity of the pointer (which is important and should not cause faults)
but on the firmware blindly reading the name buffer without even
checking the size. If size=0 it should return EFI_BUFFER_TOO_SMALL and
not EFI_NOT_FOUND, as per my understanding of the spec and as expected
by FWTS. If size is 0 and still it looks for an empty string to start
traversing the list using zero-size buffer, it would read garbage and
indeed return EFI_NOT_FOUND as it did not find that garbage name.

Thanks and BR,
Ricardo

>  
> /
>     Leif
>