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

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

  Hi,

> >> - if the StartId is 0x55aa, then we need to look further, beause we
> >> can't decide yet. For example, if State is VAR_HEADER_VALID_ONLY (0x7f),
> >> then it might be fine for the variable header (at the very end of the
> >> varstore) *not* to be followed by payload bytes (name, data).
> > 
> > Not sure this makes sense.  VAR_HEADER_VALID_ONLY is a temporary state,
> > while the variable driver writes name and data just after the header,
> > to be updated to VAR_ADDED when the write completed successfully.  So
> > I'd expect to never find a header without space for name + data.
> 
> - Do we know for sure that VAR_HEADER_VALID_ONLY is never expected to be
> seen?

Writing goes like this:

  (1) find free space
  (2) write header, with VAR_HEADER_VALID_ONLY.
  (3) write name + data
  (4) update header, set state = VAR_ADDED.

> What if the variable update design defines VAR_HEADER_VALID_ONLY
> specifically so that the variable driver can recover from a power loss
> "in the middle"?

Power loss in step (3) can surely lead to variables in
VAR_HEADER_VALID_ONLY state, and I'd expect the variable driver can
actually recover from that.

[ side note:  The (2) write should be small enough that it fits into the
              flash block write buffer (128 bytes).  Which could be
              important to maintain variable store consistency. ]

Nevertheless we should never find a header at the end of the variable
store, without space allocated for name + date.  Minimal space for the
name is 4 bytes (one char16 + '\0'), for the data 1 byte, alignment
rounds the latter to 4 bytes too, so this should be true:

VarOffset + sizeof(*VarHeader) + 8 <= VariableStoreHeader->Size

> So I figure, if we accept VAR_HEADER_VALID_ONLY in that logic, then we
> should also accept VAR_HEADER_VALID_ONLY if it's at the very end of
> the varstore.

Disagree, see above.  Storing the header at a place which leaves no room
for name + data doesn't make sense to me.

We could go the extra mile and look at the next StartId location, verify
StartId != 0x55aa, in the no-space-left-for-header case.

take care,
  Gerd



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