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

["Laszlo Ersek" <lersek@redhat.com>]

On 1/4/24 16:06, Gerd Hoffmann wrote:
>   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.

OK, that sounds convincing, thanks!
Laszlo

> 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 (#113283): https://edk2.groups.io/g/devel/message/113283
Mute This Topic: https://groups.io/mt/103171811/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-