[3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()

Commit Message

Andrew Cooper March 23, 2020, 10:17 a.m. UTC

cpu_request_microcode() needs to scan its container and duplicate one blob,
but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
Furthermore, the length checking is only safe from overflow in 64bit builds.

Delete get_next_ucode_from_buffer() and alter the purpose of the saved
variable to simply point somewhere in buf until we're ready to return.

This is only a modest reduction in absolute code size (-144), but avoids
making memory allocations for every blob in the container.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
---
 xen/arch/x86/cpu/microcode/intel.c | 69 ++++++++++++++------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

Comments

Jan Beulich March 25, 2020, 1:34 p.m. UTC | #1

On 23.03.2020 11:17, Andrew Cooper wrote:
>  static struct microcode_patch *cpu_request_microcode(const void *buf,
>                                                       size_t size)
>  {
> -    long offset = 0;
>      int error = 0;
> -    struct microcode_intel *mc, *saved = NULL;
> +    const struct microcode_intel *saved = NULL;
>      struct microcode_patch *patch = NULL;
>  
> -    while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
> +    while ( size )
>      {
> -        error = microcode_sanity_check(mc);
> -        if ( error )
> +        const struct microcode_intel *mc;
> +        unsigned int blob_size;
> +
> +        if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
> +             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
> +             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */

These two duplicate what microcode_sanity_check() does,
plus the function issues a log message when either check
fails. Since I think ...

> +             size < (blob_size =            /* Insufficient space for patch?  */
> +                     get_totalsize(&mc->hdr)) )

... this wants to come after the two version checks, how
about lifting the printk() here? The other user of
microcode_sanity_check() is an ASSERT(), where it's rather
less relevant to have such a printk().

Jan

Patch

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index f0beefe1bb..0cceac6255 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -321,75 +321,58 @@  static int apply_microcode(const struct microcode_patch *patch)
     return 0;
 }
 
-static long get_next_ucode_from_buffer(struct microcode_intel **mc,
-                                       const uint8_t *buf, unsigned long size,
-                                       unsigned long offset)
-{
-    struct microcode_header_intel *mc_header;
-    unsigned long total_size;
-
-    /* No more data */
-    if ( offset >= size )
-        return 0;
-    mc_header = (struct microcode_header_intel *)(buf + offset);
-    total_size = get_totalsize(mc_header);
-
-    if ( (offset + total_size) > size )
-    {
-        printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
-        return -EINVAL;
-    }
-
-    *mc = xmemdup_bytes(mc_header, total_size);
-    if ( *mc == NULL )
-        return -ENOMEM;
-
-    return offset + total_size;
-}
-
 static struct microcode_patch *cpu_request_microcode(const void *buf,
                                                      size_t size)
 {
-    long offset = 0;
     int error = 0;
-    struct microcode_intel *mc, *saved = NULL;
+    const struct microcode_intel *saved = NULL;
     struct microcode_patch *patch = NULL;
 
-    while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
+    while ( size )
     {
-        error = microcode_sanity_check(mc);
-        if ( error )
+        const struct microcode_intel *mc;
+        unsigned int blob_size;
+
+        if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
+             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
+             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */
+             size < (blob_size =            /* Insufficient space for patch?  */
+                     get_totalsize(&mc->hdr)) )
         {
-            xfree(mc);
+            error = -EINVAL;
             break;
         }
 
+        error = microcode_sanity_check(mc);
+        if ( error )
+            break;
+
         /*
          * If the new update covers current CPU, compare updates and store the
          * one with higher revision.
          */
         if ( (microcode_update_match(mc) != MIS_UCODE) &&
              (!saved || (mc->hdr.rev > saved->hdr.rev)) )
-        {
-            xfree(saved);
             saved = mc;
-        }
-        else
-            xfree(mc);
+
+        buf  += blob_size;
+        size -= blob_size;
     }
-    if ( offset < 0 )
-        error = offset;
 
     if ( saved )
     {
         patch = xmalloc(struct microcode_patch);
         if ( patch )
-            patch->mc_intel = saved;
-        else
         {
-            xfree(saved);
-            error = -ENOMEM;
+            patch->mc_intel = xmemdup_bytes(saved, get_totalsize(&saved->hdr));
+            if ( !patch->mc_intel )
+            {
+                XFREE(patch);
+                error = -ENOMEM;
+            }
         }
+        else
+            error = -ENOMEM;
     }
 
     if ( error && !patch )