| Message ID | 20240913110907.1902340-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/ucode: Fix buffer under-run when parsing AMD containers | expand |
On 13.09.2024 13:09, Andrew Cooper wrote: > From: Demi Marie Obenour <demi@invisiblethingslab.com> > > 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@invisiblethingslab.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> I wonder though about scan_equiv_cpu_table(): Should it perhaps complain if it doesn't find a null entry? And when it find ones, but that's not last? Jan
On 13/09/2024 1:45 pm, Jan Beulich wrote: > On 13.09.2024 13:09, Andrew Cooper wrote: >> From: Demi Marie Obenour <demi@invisiblethingslab.com> >> >> 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@invisiblethingslab.com> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > I wonder though about scan_equiv_cpu_table(): Should it perhaps complain > if it doesn't find a null entry? And when it find ones, but that's not > last? I'm erring on the "be liberal on what you accept" side of things. scan_equiv_cpu_table() is a relic of Fam10h systems (the last time that two different CPU steppings shared a ucode blob), and recent changes to Linux suggest that AMD have retired the concept. The only thing we do in scan_equiv_cpu_table() is see if there's a mapping for the current system, and we loop over all entries, even if that happens to be 0. ~Andrew
On Fri, Sep 13, 2024 at 12:09:07PM +0100, Andrew Cooper wrote: > From: Demi Marie Obenour <demi@invisiblethingslab.com> > > 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@invisiblethingslab.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Demi Marie Obenour <demi@invisiblethingslab.com> > > 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@invisiblethingslab.com>
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;