Re: [PATCH for-4.15] x86/ucode/amd: Handle length sanity check failures more gracefully

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2021 22:49, Andrew Cooper wrote:
> Currently, a failure of verify_patch_size() causes an early abort of the
> microcode blob loop, which in turn causes a second go around the main
> container loop, ultimately failing the UCODE_MAGIC check.
> 
> First, check for errors after the blob loop.  An error here is unrecoverable,
> so avoid going around the container loop again and printing an
> unhelpful-at-best error concerning bad UCODE_MAGIC.
> 
> Second, split the verify_patch_size() check out of the microcode blob header
> check.  In the case that the sanity check fails, we can still use the
> known-to-be-plausible header length to continue walking the container to
> potentially find other applicable microcode blobs.

Since the code comment you add further clarifies this, if my
understanding here is correct that you don't think we should
mistrust the entire container in such a case ...

> Before:
>   (XEN) microcode: Bad microcode data
>   (XEN) microcode: Wrong microcode patch file magic
>   (XEN) Parsing microcode blob error -22
> 
> After:
>   (XEN) microcode: Bad microcode length 0x000015c0 for cpu 0xa000
>   (XEN) microcode: Bad microcode length 0x000015c0 for cpu 0xa010
>   (XEN) microcode: Bad microcode length 0x000015c0 for cpu 0xa011
>   (XEN) microcode: Bad microcode length 0x000015c0 for cpu 0xa200
>   (XEN) microcode: Bad microcode length 0x000015c0 for cpu 0xa210
>   (XEN) microcode: Bad microcode length 0x000015c0 for cpu 0xa500
>   (XEN) microcode: couldn't find any matching ucode in the provided blob!
> 
> Fixes: 4de936a38a ("x86/ucode/amd: Rework parsing logic in 
> cpu_request_microcode()")
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

After all we're trying to balance between detecting broken
containers and having wrong constants ourselves. Personally
I'd be more inclined to err on the safe side and avoid
further loading attempts, but I can see the alternative
perspective also being a reasonable one.

Jan