xen/ucode: Make Intel's microcode_sanity_check() stricter

[PATCH] xen/ucode: Make Intel's microcode_sanity_check() stricter

Posted by Andrew Cooper 1 year, 1 month ago

From: Demi Marie Obenour <demi@invisiblethingslab.com>

The SDM states that data size must be a multiple of 4, but Xen doesn't check
this propery.

This is liable to cause a later failures, but should be checked explicitly.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/cpu/microcode/intel.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 6f6957058684..bad51f64724a 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -155,10 +155,13 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
     uint32_t sum;
 
     /*
-     * Total size must be a multiple of 1024 bytes.  Data size and the header
-     * must fit within it.
+     * The SDM states:
+     * - Data size must be a multiple of 4.
+     * - Total size must be a multiple of 1024 bytes.  Data size and the
+     *   header must fit within it.
      */
     if ( (total_size & 1023) ||
+         (data_size & 3) ||
          data_size > (total_size - MC_HEADER_SIZE) )
     {
         printk(XENLOG_WARNING "microcode: Bad size\n");
-- 
2.39.2

Re: [PATCH] xen/ucode: Make Intel's microcode_sanity_check() stricter

Posted by Jan Beulich 1 year, 1 month ago

On 13.09.2024 16:21, Andrew Cooper wrote:
> From: Demi Marie Obenour <demi@invisiblethingslab.com>
> 
> The SDM states that data size must be a multiple of 4, but Xen doesn't check
> this propery.
> 
> This is liable to cause a later failures, but should be checked explicitly.
> 
> 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>

> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -155,10 +155,13 @@ static int microcode_sanity_check(const struct microcode_patch *patch)
>      uint32_t sum;
>  
>      /*
> -     * Total size must be a multiple of 1024 bytes.  Data size and the header
> -     * must fit within it.
> +     * The SDM states:
> +     * - Data size must be a multiple of 4.
> +     * - Total size must be a multiple of 1024 bytes.  Data size and the
> +     *   header must fit within it.
>       */
>      if ( (total_size & 1023) ||
> +         (data_size & 3) ||
>           data_size > (total_size - MC_HEADER_SIZE) )

And luckily get_totalsize() guarantees total_size > 0, for this
subtraction not to underflow. Maybe worth also mentioning in the
comment as you adjust it anyway.

Jan