[PATCH 0/6] Miscellaneous FWU fixes

[Michal Simek]

On 9/5/24 08:24, Sughosh Ganu wrote:
> On Wed, 4 Sept 2024 at 17:30, Michal Simek <michal.simek at amd.com> wrote:
>>
>>
>>
>> On 8/30/24 13:40, Sughosh Ganu wrote:
>>>
>>> The following set of patches are miscellaneous fixes and some
>>> hardening of the FWU update logic.
>>>
>>> Sughosh Ganu (6):
>>>     fwu: v2: perform some checks before reading metadata
>>>     fwu: v2: try reading both copies of metadata
>>>     fwu: v1: do a version check for the metadata
>>>     fwu: check all images for transitioning out of Trial State
>>>     fwu: add dependency checks for selecting FWU metadata version
>>>     fwu: do not allow capsule processing on exceeding Trial Counter
>>>       threshold
>>>
>>>    include/fwu.h            | 11 ++++++
>>>    lib/fwu_updates/Kconfig  |  1 +
>>>    lib/fwu_updates/fwu.c    | 31 +++++++++++++++-
>>>    lib/fwu_updates/fwu_v1.c | 18 +++++++--
>>>    lib/fwu_updates/fwu_v2.c | 80 ++++++++++++++++++++++------------------
>>>    5 files changed, 99 insertions(+), 42 deletions(-)
>>>
>>
>> I found one more thing.
>>
>> I did this change
>>
>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
>> index fbc2067bc12d..dab9530e499c 100644
>> --- a/tools/mkfwumdata.c
>> +++ b/tools/mkfwumdata.c
>> @@ -293,7 +293,7 @@ static void fwu_fill_version_specific_mdata(struct
>> fwu_mdata_object *mobj)
>>           struct fwu_mdata *mdata = mobj->mdata;
>>
>>           mdata->metadata_size = mobj->size;
>> -       mdata->desc_offset = sizeof(struct fwu_mdata);
>> +       mdata->desc_offset = 0x21; sizeof(struct fwu_mdata);
>>
>>           for (i = 0; i < MAX_BANKS_V2; i++)
>>                   mdata->bank_state[i] = i < mobj->banks ?
>>
>>
>> to break desc_offset location. I generated mdata structure and write it to
>> primary mdata partition.
>>
>> This has been spotted by (my debug) which is the reason why I did it.
>>
>>          if (g_mdata.desc_offset != FWU_IMG_DESC_OFFSET) {
>>                  printf("mdata offset is not 0x20\n");
>>                  return false;
>>          }
>>
>> But I got one more message below which
>>
>> mdata offset is not 0x20
>> Both FWU metadata copies are valid but do not match. Restoring the secondary
>> partition from the primary
> 
> The reason why this logic is in place is to handle the scenario
> whereby, during the metadata update, the primary copy gets updated
> correctly, but due to some reason (like a power cut), the secondary
> copy does not -- this is because the primary copy gets updated first.
> In such a scenario, the secondary copy of metadata can be then
> restored from the primary copy. In the scenario where the primary copy
> is corrupted, it will show up in the crc check, and will get restored
> from the secondary copy.

But in this case - CRC of primary is correct but data inside is wrong because 
offset is at 0x21 not at 0x20.
That likely means that U-Boot is not actually using this field to find where 
data is but I think that's fine to say not supported mdata structure but u-boot 
should never copy this "primary broken" description to secondary one as syncup.

M