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

[Leif Lindholm]

On Wed, Mar 11, 2015 at 03:32:33PM -0700, Ricardo Neri wrote:
> > > 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.

OK, that makes more sense.

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

Well, the problem is that it _can_ tell the difference
between ZERO_SIZE_PTR - from include/linux/slab.h:
#define ZERO_SIZE_PTR ((void *)16)

Hence the NULL pointer check fails.
 
> > 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.

I am not saying that it would not make sense for the API to do that -
I am saying that this is not what the API specifies. There is even an
explicit note in there that says:
"Passing in a VariableName parameter that is neither a Null-terminated
string nor a value that was returned on the previous call to
GetNextVariableName() may also produce unpredictable results."

Regards,

Leif