| Message ID | 06edbbb7831620bc04c6453b95aad80eaebb20a1.1726162000.git.demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix two problems in the microcode parsers | expand |
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@invisiblethingslab.com> > --- > 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
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@invisiblethingslab.com> >> --- >> 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
On Thu, Sep 12, 2024 at 07:44:05PM +0100, Andrew Cooper wrote: > 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@invisiblethingslab.com> > > --- > > 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? The problem is that (et->len / sizeof(et->eq[0])) - 1 will underflow, causing Xen to access et->eq[UINT32_MAX], which will (hopefully) crash. The simplest solution is to drop that check, since scan_equiv_cpu_table() already checks for terminating entries. > 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. Correct. Anything nonzero will either be greater than MC_HEADER_SIZE or will be rejected due to (total_size & 1023) not being zero. That said, the new code is much more obviously correct. > 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 It's still undefined behavior (cast to an unaligned pointer).
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 ) { 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 ) { printk(XENLOG_WARNING "microcode: Bad size\n"); return -EINVAL;
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. Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> --- xen/arch/x86/cpu/microcode/amd.c | 1 + xen/arch/x86/cpu/microcode/intel.c | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) base-commit: 035baa203b978b219828d0d3c16057beb344f35c