[PATCH 1/3] uefirtvariable: Check new VariableNameSize from GetNextVariableName()

[Matt Fleming]

On Wed, 2013-03-06 at 11:32 +0000, Colin Ian King wrote:
> On 06/03/13 11:27, Matt Fleming wrote:
> > On Tue, 2013-03-05 at 21:54 +0000, Matt Fleming wrote:
> >> From: Matt Fleming <matt.fleming at intel.com>
> >>
> >> Some firmware implementations update VariableNameSize in
> >> GetNextVariableName() with a value that is larger than the actual
> >> buffer required to hold the VariableName string. This is not
> >> technically a bug, but most implementations do update VariableNameSize
> >> with the value of strlen(VariableName) + 1, so print a warning if a
> >> different value is found.
> >>
> >> Signed-off-by: Matt Fleming <matt.fleming at intel.com>
> >> ---
> >>   src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
> >>   1 file changed, 72 insertions(+), 2 deletions(-)
> >
> > Folks, I was fairly certain that there were no implementations in the
> > wild that failed to update VariableNameSize on EFI_SUCCESS, but I've
> > just been informed that there are some.
> 
> Urgh, is this because the spec is a tad ambiguous that we're seeing this 
> in the wild?

Yeah. The GetNextVariableName() documentation in Section 7.2 of the UEFI
2.3.1 spec doesn't say what should happen to VariableNameSize in the
case of EFI_SUCCESS. It only says that it should be updated to the size
of the buffer required to hold VariableName in the EFI_BUFFER_TOO_SMALL
case.

But it's worth pointing out that upstream EDK2 does update
VariableNameSize with the size of VariableName on every invocation,
which means it's likely that firmware that also has this behaviour is
based on EDK2 or some derivative. Any implementation that doesn't is
probably of the older, weirder variety.
 
> >
> > You may or may not want to take this patch, since it is warning about an
> > undocumented behaviour - albeit one that many implementations exhibit.
> >
> 
> One of the remits of fwts is to catch ambiguous behaviour so firmware 
> can be fixed before it gets released.  Will catching this kind of 
> behaviour be useful? If so, there seems merit to keeping it.

The bug that this test was designed to catch is one affecting HP
machines where VariableNameSize is updated on every invocation, but the
updated value is neither the size of VariableName nor the initial value
of VaraibleNameSize - it's some value in the middle and I'm not quite
sure what it is supposed to represent.

I think there's merit in catching that particular issue, but probably
not in warning if VariableNameSize isn't updated at all. I'll send a v2
of this patch with those changes.

-- 
Matt Fleming, Intel Open Source Technology Center