Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

["Gerd Hoffmann" <kraxel@redhat.com>]

On Thu, Dec 07, 2023 at 05:16:10PM +0100, Ard Biesheuvel wrote:
> Hi Gerd,
> 
> On Thu, 7 Dec 2023 at 10:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > Extend the ValidateFvHeader function, additionally to the header checks
> > walk over the list of variables and sanity check them.
> >
> > In case we find inconsistencies indicating variable store corruption
> > return EFI_NOT_FOUND so the variable store will be re-initialized.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> This seems like a good idea to me, and the change looks generally
> fine, except for one nit below:

> > +      case VAR_HEADER_VALID_ONLY:
> > +        VarState = "header-ok";
> > +        VarName  = L"<unknown>";
> > +        break;
> > +      case VAR_ADDED:
> > +        VarState = "ok";
> > +        break;
> > +      case VAR_ADDED &VAR_IN_DELETED_TRANSITION:
> 
> This looks odd, so please add a comment explaining why these constants
> are constructed this way.

The VAR_HEADER_VALID_ONLY and VAR_ADDED states are applied using an
assignment, i.e. "State = VAR_...".

The other two are used to clear bits, i.e. "State &= VAR_...".

So the usual livecycle for the State field is:

  (1) 0x7f (VAR_HEADER_VALID_ONLY)
  (2) 0x3f (VAR_ADDED)
  (3) 0x3e (VAR_ADDED & VAR_IN_DELETED_TRANSITION)
  (4) 0x3c (VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED)

Seems it can also happen that variable entries jump directly from (2) to
(4), resulting in 0x3d (VAR_ADDED & VAR_DELETED) being an possible
alternative value for (4).

I'm not fully sure what happens to VAR_HEADER_VALID_ONLY entries, i.e.
whenever they go through the (3) + (4) deletion cycles too or whenever
they are garbage-collected in some other way.  I suspect the latter,
given that the reclaim logic in the variable driver looks at the
variable names, but VAR_HEADER_VALID_ONLY entries don't have a valid
variable name.


BTW: Is there any good documentation on the design of the variable
driver and the fault tolerant flash writes?  Is the variable flash
supposed to be in a consistent state at any given time, even if
interrupted in the middle of some update operation?

The qemu flash emulation is a bit sloppy, specifically block writes can
be half-completed when a reset happens in the middle of the operation.
Which maybe is the root cause for varstore corruption.

On the other hand the edk2 flash code does not look like it carefully
chooses blocks when updating flash, so I'm not sure it actually depends
on atomic block updates to ensure consistency.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112226): https://edk2.groups.io/g/devel/message/112226
Mute This Topic: https://groups.io/mt/103031342/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-