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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.04.14 at 21:54, <aravind.gopalakrishnan@xxxxxxx> wrote:
> Each family has a stipulated max patch_size. Use this as
> additional sanity check before we apply it.

And iirc there was a size limit check earlier, and it got dropped for one
reason or another - did you check history?

> @@ -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)
> +    {
> +    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;

Please simply "return patch_size <= max_size" in cases like this.

> @@ -329,6 +369,11 @@ static int cpu_request_microcode(int cpu, const void 
> *buf, size_t bufsize)
>  
>          last_offset = offset;
>  
> +        if ( error == -EEXIST ) {

Coding style, but as Andrew already indicated this block of code isn't
correct anyway.

> @@ -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;

Is it really correct for this to get switched from success to error return?

> @@ -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;

Bogus (pointless) change?

> @@ -408,7 +456,7 @@ err2:
>  err1:
>      xfree(mc_amd);
>      uci->mc.mc_amd = NULL;
> -    return -ENOMEM;
> +    return error;

Same here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel