[v2] Fix two problems in the microcode parsers

Commit Message

Demi Marie Obenour Sept. 12, 2024, 9:11 p.m. UTC

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 <[e-mail]>
---
 xen/arch/x86/cpu/microcode/amd.c   | 6 +++---
 xen/arch/x86/cpu/microcode/intel.c | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)


base-commit: 035baa203b978b219828d0d3c16057beb344f35c

Comments

Jan Beulich Sept. 13, 2024, 7:02 a.m. UTC | #1

On 12.09.2024 23:11, 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.

Okay, this explains the background. But nothing is said about what's
actually wrong with the code that's being changed.

As to the subject: Please have it have a proper prefix.

> --- 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;
> @@ -365,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 )

What's this literal 4? Wants to be some alignof(), sizeof(), or alike
imo.

> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -149,8 +149,8 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
>  {
>      const struct extended_sigtable *ext;
>      const uint32_t *ptr;
> -    unsigned int total_size = get_totalsize(patch);
> -    unsigned int data_size = get_datasize(patch);
> +    uint32_t total_size = get_totalsize(patch);
> +    uint32_t data_size = get_datasize(patch);

I see no reason for moving in the wrong direction here, as far as
./CODING_STYLE goes. If this is truly needed,it needs justifying in
the description.

> @@ -159,7 +159,8 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
>       * must fit within it.
>       */
>      if ( (total_size & 1023) ||
> -         data_size > (total_size - MC_HEADER_SIZE) )
> +         data_size > (total_size - MC_HEADER_SIZE) ||
> +         (data_size % 4) != 0 )

See above as to the use of literal numbers.

Jan

Roger Pau Monné Sept. 13, 2024, 7:47 a.m. UTC | #2

On Thu, Sep 12, 2024 at 05:11:32PM -0400, 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.

As said by Jan I think this needs expanding as to what's actually
being fixed, to give readers context.

Additionally you want to add one or more "Fixes" tags if this is a
bugfix.

Thanks, Roger.

Patch

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index d2a26967c6dbc4695602dd46d5836a6d88e15072..08fe3ac61c18a8e16f694e128973da96ce6995e3 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;
@@ -365,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;
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 6f6957058684d7275d62e525e88ff678db9eb6d2..3e113c84b1fff0ba18a0251dbac0c7d6e2b229f6 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -149,8 +149,8 @@  static int microcode_sanity_check(const struct microcode_patch *patch)
 {
     const struct extended_sigtable *ext;
     const uint32_t *ptr;
-    unsigned int total_size = get_totalsize(patch);
-    unsigned int data_size = get_datasize(patch);
+    uint32_t total_size = get_totalsize(patch);
+    uint32_t data_size = get_datasize(patch);
     unsigned int i, ext_size;
     uint32_t sum;
 
@@ -159,7 +159,8 @@  static int microcode_sanity_check(const struct microcode_patch *patch)
      * must fit within it.
      */
     if ( (total_size & 1023) ||
-         data_size > (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;