[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/ucode: Fix buffer under-run when parsing AMD containers
On Fri, Sep 13, 2024 at 12:09:07PM +0100, Andrew Cooper wrote: > From: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> > > The AMD container format has no formal spec. It is, at best, precision > guesswork based on AMD's prior contributions to open source projects. The > Equivalence Table has both an explicit length, and an expectation of having a > NULL entry at the end. > > Xen was sanity checking the NULL entry, but without confirming that an entry > was present, resulting in a read off the front of the buffer. With some > manual debugging/annotations this manifests as: > > (XEN) *** Buf ffff83204c00b19c, eq ffff83204c00b194 > (XEN) *** eq: 0c 00 00 00 44 4d 41 00 00 00 00 00 00 00 00 00 aa aa aa aa > ^-Actual buffer-------------------^ > (XEN) *** installed_cpu: 000c > (XEN) microcode: Bad equivalent cpu table > (XEN) Parsing microcode blob error -22 > > When loaded by hypercall, the 4 bytes interpreted as installed_cpu happen to > be the containing struct ucode_buf's len field, and luckily will be nonzero. > > When loaded at boot, it's possible for the access to #PF if the module happens > to have been placed on a 2M boundary by the bootloader. Under Linux, it will > commonly be the end of the CPIO header. > > Drop the probe of the NULL entry; Nothing else cares. A container without one > is well formed, insofar that we can still parse it correctly. With this > dropped, the same container results in: > > (XEN) microcode: couldn't find any matching ucode in the provided blob! > > Fixes: 4de936a38aa9 ("x86/ucode/amd: Rework parsing logic in > cpu_request_microcode()") > Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > --- > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> > CC: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> > > Split out of joint patch, and analyse. > > I couldn't trigger any of the sanitisers with this, hence the manual > debugging. > > This patch intentionally doesn't include patch 2's extra hunk changing: > > @@ -364,7 +364,8 @@ static struct microcode_patch *cf_check > cpu_request_microcode( > if ( size < sizeof(*mc) || > (mc = buf)->type != UCODE_UCODE_TYPE || > size - sizeof(*mc) < mc->len || > - mc->len < sizeof(struct microcode_patch) ) > + mc->len < sizeof(struct microcode_patch) || > + mc->len % 4 != 0 ) > { > printk(XENLOG_ERR "microcode: Bad microcode data\n"); > error = -EINVAL; > > Intel have a spec saying the length is mutliple of 4. AMD do not, and have > microcode which genuinely isn't a multiple of 4. In this case the structs at the top should be __attribute__((packed)) to avoid undefined behavior. That can be a separate patch, though. > --- > xen/arch/x86/cpu/microcode/amd.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/xen/arch/x86/cpu/microcode/amd.c > b/xen/arch/x86/cpu/microcode/amd.c > index d2a26967c6db..32490c8b7d2a 100644 > --- a/xen/arch/x86/cpu/microcode/amd.c > +++ b/xen/arch/x86/cpu/microcode/amd.c > @@ -338,8 +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->eq[(et->len / sizeof(et->eq[0])) - 1].installed_cpu ) > + et->len % sizeof(et->eq[0]) ) > { > printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n"); > error = -EINVAL; > -- > 2.39.2 > Reviewed-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab Attachment:
signature.asc
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |