[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] Fix two problems in the microcode parsers
On 12/09/2024 6:39 pm, Demi Marie Obenour wrote: > The microcode might come from a questionable source, so it is necessary > for the parsers to treat it as untrusted. The CPU will validate the > microcode before applying it, so loading microcode from unofficial > sources is actually a legitimate thing to do in some cases. While true, the better argument is that ucode blobs are mostly encrypted data, so a mis-step tends to fall over a very long way. > > 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. > diff --git a/xen/arch/x86/cpu/microcode/intel.c > b/xen/arch/x86/cpu/microcode/intel.c > index > 6f6957058684d7275d62e525e88ff678db9eb6d2..7a383adbdf1b5cb58f2e4c89e3a1c11ecc053993 > 100644 > --- a/xen/arch/x86/cpu/microcode/intel.c > +++ b/xen/arch/x86/cpu/microcode/intel.c > @@ -158,8 +158,9 @@ static int microcode_sanity_check(const struct > microcode_patch *patch) > * Total size must be a multiple of 1024 bytes. Data size and the header > * must fit within it. > */ > - if ( (total_size & 1023) || > - data_size > (total_size - MC_HEADER_SIZE) ) > + if ( (total_size & 1023) || (total_size < MC_HEADER_SIZE) || > + data_size > (total_size - MC_HEADER_SIZE) || > + (data_size % 4) != 0 ) The caller has already checked some properties. Furthermore, total_size of 0 in the header is a special case for pre-Pentium4 ucode and is substituted with a constant, so can never be 0 in the variable here. Therefore, I don't think the (total_size < MC_HEADER_SIZE) check can ever fail. data_size being 4-byte aligned probably should be checked, although I think the later cross-check (ext_sigtable fits exactly in the remaining space) should catch this before the function wanders off the buffer. ~Andrew
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |