Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/04/14 20:54, Aravind Gopalakrishnan wrote:
> Each family has a stipulated max patch_size. Use this as
> additional sanity check before we apply it.
>
> Also, if we find current patch level equal or higher, then we
> return early from microcode_fits, but we don't do anything about it.
> This means we waste a bit of boot time needlessly parsing remainder
> of firmware image.
>
> This patch returns EEXIST when above situation occurs so that-
> If we do apply patch during presmp_init, then we save bit of time
> when these routines are invoked during microcode_init.
> If there is nothing to apply during presmp_init, then we flush
> out ucode_blob.size and ucode_blob.data anyway since we are
> already at required level.
>
> Since 'microcode_fits' returns int now, modify error machinery
> to return EINVAL for error and '0' for success.
>
> While at it, fix comment at very top to indicate we support ucode
> patch loading from fam10h and higher.
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> Reviewed-by: Tom Lendacky <Thomas.Lendacky@amd.com>
> Reviewed-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  xen/arch/x86/microcode_amd.c |   70 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 59 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index b227173..6fafe85 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -8,7 +8,7 @@
>   *  Tigran Aivazian <tigran@aivazian.fsnet.co.uk>
>   *
>   *  This driver allows to upgrade microcode on AMD
> - *  family 0x10 and 0x11 processors.
> + *  family 0x10 and later.
>   *
>   *  Licensed unter the terms of the GNU General Public
>   *  License version 2. See file COPYING for details.
> @@ -94,7 +94,40 @@ static int collect_cpu_info(int cpu, struct cpu_signature *csig)
>      return 0;
>  }
>  
> -static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
> +static bool_t verify_patch_size(uint32_t patch_size)
> +{
> +    uint8_t family;
> +    uint32_t max_size;
> +
> +#define F1XH_MPB_MAX_SIZE 2048
> +#define F14H_MPB_MAX_SIZE 1824
> +#define F15H_MPB_MAX_SIZE 4096
> +#define F16H_MPB_MAX_SIZE 3458
> +
> +    family = boot_cpu_data.x86;
> +    switch (family)

You can drop the family variable and just switch on boot_cpu_data.x86

> +    {
> +    case 0x14:
> +        max_size = F14H_MPB_MAX_SIZE;
> +        break;
> +    case 0x15:
> +        max_size = F15H_MPB_MAX_SIZE;
> +        break;
> +    case 0x16:
> +        max_size = F16H_MPB_MAX_SIZE;
> +        break;
> +    default:
> +        max_size = F1XH_MPB_MAX_SIZE;
> +        break;
> +    }
> +
> +    if ( patch_size > max_size )
> +        return 0;
> +
> +    return 1;
> +}
> +
> +static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>  {
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>      const struct microcode_header_amd *mc_header = mc_amd->mpb;
> @@ -118,19 +151,25 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
>      }
>  
>      if ( !equiv_cpu_id )
> -        return 0;
> +        return -EINVAL;
>  
>      if ( (mc_header->processor_rev_id) != equiv_cpu_id )
> -        return 0;
> +        return -EINVAL;
> +
> +    if ( !verify_patch_size(mc_amd->mpb_size) )
> +    {
> +        printk(KERN_ERR "microcode: patch size mismatch\n");
> +        return -EINVAL;

XENLOG_ERR "microcode patch size too large"
return -E2BIG;

And is this really worth being an error as opposed to a warning, or
frankly even just debug? It is presumably one of N possible blobs in the
hypercall.

> +    }
>  
>      if ( mc_header->patch_id <= uci->cpu_sig.rev )
> -        return 0;
> +        return -EEXIST;
>  
>      printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
>             "update with version %#x (current=%#x)\n",
>             cpu, mc_header->patch_id, uci->cpu_sig.rev);
>  
> -    return 1;
> +    return 0;
>  }
>  
>  static int apply_microcode(int cpu)
> @@ -319,7 +358,8 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>      while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>                                                 &offset)) == 0 )
>      {
> -        if ( microcode_fits(mc_amd, cpu) )
> +        error = microcode_fits(mc_amd, cpu);
> +        if ( !error )
>          {
>              error = apply_microcode(cpu);
>              if ( error )
> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>  
>          last_offset = offset;
>  
> +        if ( error == -EEXIST ) {
> +            printk(KERN_DEBUG "microcode: patch is already at required level or greater.\n");
> +            break;

You can't break from the loop here.  What if a subsequent blob in the
buffer contains a newer piece of microcode?

~Andrew

> +        }
> +
>          if ( offset >= bufsize )
>              break;
>      }
> @@ -370,9 +415,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>      struct microcode_amd *mc_amd = uci->mc.mc_amd;
>      const struct microcode_amd *src = mc;
> +    int error;
>  
> -    if ( !microcode_fits(src, cpu) )
> -        return 0;
> +    error = microcode_fits(src, cpu);
> +    if ( error )
> +        return error;
>  
>      if ( src != mc_amd )
>      {
> @@ -383,10 +430,11 @@ static int microcode_resume_match(int cpu, const void *mc)
>              xfree(mc_amd);
>          }
>  
> +        error = -ENOMEM;
>          mc_amd = xmalloc(struct microcode_amd);
>          uci->mc.mc_amd = mc_amd;
>          if ( !mc_amd )
> -            return -ENOMEM;
> +            return error;
>          mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
>          if ( !mc_amd->equiv_cpu_table )
>              goto err1;
> @@ -408,7 +456,7 @@ err2:
>  err1:
>      xfree(mc_amd);
>      uci->mc.mc_amd = NULL;
> -    return -ENOMEM;
> +    return error;
>  }
>  
>  static int start_update(void)