Re: [PATCH] Fix two problems in the microcode parsers

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/09/2024 7:44 pm, Andrew Cooper wrote:
> On 12/09/2024 6:39 pm, Demi Marie Obenour wrote:
>> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/x86/cpu/microcode/amd.c   | 1 +
>>  xen/arch/x86/cpu/microcode/intel.c | 5 +++--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/microcode/amd.c 
>> b/xen/arch/x86/cpu/microcode/amd.c
>> index 
>> d2a26967c6dbc4695602dd46d5836a6d88e15072..31ee5717c5f1c7d0b7e29d990cf4d1024d775900
>>  100644
>> --- a/xen/arch/x86/cpu/microcode/amd.c
>> +++ b/xen/arch/x86/cpu/microcode/amd.c
>> @@ -338,6 +338,7 @@ static struct microcode_patch *cf_check 
>> cpu_request_microcode(
>>          if ( size < sizeof(*et) ||
>>               (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE ||
>>               size - sizeof(*et) < et->len ||
>> +             et->len < sizeof(et->eq[0]) ||
>>               et->len % sizeof(et->eq[0]) ||
>>               et->eq[(et->len / sizeof(et->eq[0])) - 1].installed_cpu )
>>          {
> This is complicated by AMD having no spec in the slightest on their
> container format, but this change makes Xen reject a case that I
> reasoned was well formed (if unwise).
>
> IMO, the following binary is well formed:
>
>     0x00414d44 /* MAGIC */
>     0x00000000 /* type=equiv */
>     0x00000000 /* len=0, == no entries */
>                /* no equiv entries */
>                /* no ucode entries */
>     0x00414d44 /* MAGIC */
>     ...
>
> and I believe that Xen would parse it correctly.  Unless you think
> there's another case I've failed to consider?
>
> AFACT the check you put in rejects the len < "1 entry" case.
>
> There is no shortage of ambiguity here; the equiv table has both an
> explicit length, and commonly has a NULL entry at the end.  But, we can
> parse with a length of 0.

Bah, and just after sending, I've spotted the problem.  When len is 0,
the installed_cpu check (which is looking for the NULL entry) moves off
the front of the buffer.

I'm very tempted to first wire this into OSS_FUZZ, seeing as we've just
got that working for the emulator.

~Andrew