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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Ian Jackson" <iwj@xenproject.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH for-4.15] x86/ucode/amd: Handle length sanity check failures more gracefully
Date: Wed, 10 Feb 2021 12:09:47 +0000	[thread overview]
Message-ID: <7a91591c-5401-2419-c636-7e456bcfe911@citrix.com> (raw)
In-Reply-To: <87d6982a-00d9-3daa-ebd7-9afb8ee60126@suse.com>

On 10/02/2021 10:55, Jan Beulich wrote:
> 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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> 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.

The more I learn, the more I'm starting to mistrust the containers.

But as we don't know whether it is us or the container at fault - and
have an example where we are at fault - I don't think blocking loading
is an appropriate thing to do.  (Amongst other things, it totally kills
any ability to test this interface.)

~Andrew