diff mbox series

[for-4.15] x86/ucode/amd: Fix OoB read in cpu_request_microcode()

Message ID 20210209234019.3827-1-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series [for-4.15] x86/ucode/amd: Fix OoB read in cpu_request_microcode() | expand

Commit Message

Andrew Cooper Feb. 9, 2021, 11:40 p.m. UTC
verify_patch_size() is a maximum size check, and doesn't have a minimum bound.

If the microcode container encodes a blob with a length less than 64 bytes,
the subsequent calls to microcode_fits()/compare_header() may read off the end
of the buffer.

Fixes: 4de936a38a ("x86/ucode/amd: Rework parsing logic in cpu_request_microcode()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>

In practice, processor_rev_id is the only field read, which is 2 bytes at
offset 24 into the header.  Not that this makes the bug any less bad.

For 4.15.  Only dom0 can load new microcode, hence no XSA, but the bug is bad
and the fix simple and obvious.
---
 xen/arch/x86/cpu/microcode/amd.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jan Beulich Feb. 10, 2021, 11 a.m. UTC | #1
On 10.02.2021 00:40, Andrew Cooper wrote:
> verify_patch_size() is a maximum size check, and doesn't have a minimum bound.
> 
> If the microcode container encodes a blob with a length less than 64 bytes,
> the subsequent calls to microcode_fits()/compare_header() may read off the end
> of the buffer.
> 
> 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>

> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -349,6 +349,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
>              if ( size < sizeof(*mc) ||
>                   (mc = buf)->type != UCODE_UCODE_TYPE ||
>                   size - sizeof(*mc) < mc->len ||
> +                 mc->len < sizeof(struct microcode_patch) ||

I was inclined to suggest to use <= here, but I guess a blob
with 1 byte of data is as bogus as one with 0 bytes of data.

Jan
Andrew Cooper Feb. 10, 2021, 12:58 p.m. UTC | #2
On 10/02/2021 11:00, Jan Beulich wrote:
> On 10.02.2021 00:40, Andrew Cooper wrote:
>> verify_patch_size() is a maximum size check, and doesn't have a minimum bound.
>>
>> If the microcode container encodes a blob with a length less than 64 bytes,
>> the subsequent calls to microcode_fits()/compare_header() may read off the end
>> of the buffer.
>>
>> 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.

>
>> --- a/xen/arch/x86/cpu/microcode/amd.c
>> +++ b/xen/arch/x86/cpu/microcode/amd.c
>> @@ -349,6 +349,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
>>              if ( size < sizeof(*mc) ||
>>                   (mc = buf)->type != UCODE_UCODE_TYPE ||
>>                   size - sizeof(*mc) < mc->len ||
>> +                 mc->len < sizeof(struct microcode_patch) ||
> I was inclined to suggest to use <= here, but I guess a blob
> with 1 byte of data is as bogus as one with 0 bytes of data.

Yeah - its unfortunate.  On the Intel side, we've got a fairly rigorous
set of sanity checks we can perform on the blob, and on the AMD side we
appear to have nothing.

This at least prevents our minimal header checks from causing OoB accesses.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index c4ab395799..cf5947389f 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -349,6 +349,7 @@  static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
             if ( size < sizeof(*mc) ||
                  (mc = buf)->type != UCODE_UCODE_TYPE ||
                  size - sizeof(*mc) < mc->len ||
+                 mc->len < sizeof(struct microcode_patch) ||
                  (!skip_ucode && !verify_patch_size(mc->len)) )
             {
                 printk(XENLOG_ERR "microcode: Bad microcode data\n");