[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 10 Feb 2021 12:09:47 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=xVhcAl44TWDzt7zo21fEbPet6O5DdizLMhnWHNpIo6I=; b=iIFZp7CL4yYc4PTWZuq+huKJXCINb7nnzt8Gj+Gb6Sm+leoDrq8Dy4IuBgo5nKbjD7b7y/N06U60Mzzs+gnfCDrywd7mxTKrDJ+Pac7zLXPkTel7mu/QcynO3tuikNrQ/mAeB28XIDRPiQYLPlDv5lGhCeIKHcjlOhXNO7tElg6er6qXEW4vNIyqAg8rV+mbTs7+SQMZzXN+AgqaEBAEIttPqzmsPZAVWFqZ1jENPDmkfyy1a8IDhwHbUkr3TpnPrgHLyUxzXLiTM+ZsMgr7FrbOoLQCdRSwxVlkJNxayHEAeEsPaelUuS3XSOcXPnJ++Xlk1rsCuYLbC2nFtTxpug==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Q+QR3Jx///i99qgMlIZBsrHx4z8azwmwxkXbJ38zLTYnWtTFcfAUPqeW0Yn9lpNn0V/lfSTqXHwF/6snSbNrlj5GfF0q8jdvI4O2sODPzj368elW9ya+P+oM4QK3g+tCaaWby+WD/MXavszylEBD9zueS06K5+IMQRtbVjogDRgg/S5SZMcN+lD/POHNGhfYpNCqrkBp3QQnH02KCjlvl6NIT5VHUVKwHvZjBUctMKO8td2nfEYb8KqToYYqhSasLoi4wvvt93J01zGeDZ7eVVJm2v7o/QdFGA11YlZ2zZwe1aVGQR1KGgmQ6huxmv7TWDuJbjbXzXTm0eVAjLnvEQ==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 10 Feb 2021 12:10:26 +0000
  • Ironport-sdr: Tp//KLyitTQZwlNHM4U+Mtql86HlxFVvM0Qq5bXzp1YOiLnFsV2zrC/ewIfKghfc/rDxL3C7Mw 0xg6+o8ossfN4epDHvoeznf1lxLfp/uXH1klZu6jlz55iaZSry1qEaFqxvHiXY+SZfbJ6IkRXN XrUZ6pMUjq2oOrnOBfrVoNO8R17qkI/g3aL8FXd5giv3mtSYzrWQkGIyQhzcNz5xXbkvgohfhj VAatX9Hrc/WvBlrIronvjCUu2lOlVkHeDpUCNCMhkZ2aFwiaCXD0TxE/NVU7PTDTjhDmYX0YF9 viA=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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

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




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.