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

[Ricardo Neri]

On Wed, 2015-03-11 at 23:12 +0000, Leif Lindholm wrote:
> 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)

Yes, the kernel can but there is no reason for which the firmware should
know the difference. I think that the kernel code handling the calls to
the firmware should translate ZERO_SIZE_PTRs into NULLs as they are
equivalent for the firmware and all its validity checks will be based on
NULL.

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

But it also says that EFI_BUFFER_TOO_SMALL should be returned if
VariableNameSize is too small for the result; VariableNameSize=0 is
clearly too small for any variable.

Thanks and BR,
Ricardo

> 
> Regards,
> 
> Leif