Change in ...coreboot[master]: soc/intel/basecode: Add support for updating ucode loaded via FIT

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27369 )

Change subject: soc/intel/basecode: Add support for updating ucode loaded via FIT
......................................................................

Patch Set 38:

(1 comment)

The implementation looks good. Only the update condition
that Patrick R. already noticed seems concerning.

https://review.coreboot.org/#/c/27369/30/src/soc/intel/common/basecode/fw_up... 
File src/soc/intel/common/basecode/fw_update/ucode_update.c:

https://review.coreboot.org/#/c/27369/30/src/soc/intel/common/basecode/fw_up... 
PS30, Line 163: 	if (staging_rev != current_ucode || current_ucode != slot_rev ||
> in a scenario where the header (which has ucode revision) is written completely but the ucode data i […]

If you want to handle a corrupted RW ucode this way, you'll need
more safety guards. I mean check for exactly that condition
`TS_ENABLE && staging_rev == slot_rev && current_ucode == slot_rev`
Otherwise, you may rewrite the staging ucode for spurious reasons.
For instance, think about what would happen if TS doesn't stick
for some reasons, e.g. battery failure and somebody implemented
ucode_update_reboot() as a cold boot. You could be running into
a reboot loop constantly erasing and rewriting the staging ucode.

You could avoid this by explicitly checking before writing if
slot_microcode and staging microcoding really differ.

Beside that one of the checks is really superfluous, the current
`if` is the equivalent of:

if (staging_rev != current_ucode)
            version_mismatch = 1;
    else /* staging_rev == current_ucode */
         if (current_ucode != slot_rev)
            version_mismatch = 1;
    else /* staging_rev == current_ucode && current_ucode == slot_rev */
         if (staging_rev != slot_rev) /* impossible */
            version_mismatch = 1;

I guess the control flow could be

if (staging_rev != slot_rev || current_ucode != slot_rev) {
            ...
            if (!memcmp(..., slot_microcode, get_microcode_size(...)))
                    perform_update(slot_microcode);
    }

Or slightly optimized usual case:

if (staging_rev != slot_rev) {
            perform_update(slot_microcode);
    } else if (current_ucode != slot_rev) {
            ...
            if (!memcmp(..., slot_microcode, get_microcode_size(...)))
                    perform_update(slot_microcode);
    }