Re: [PATCH 02/13] x86/microcode/intel: Do the mc_saved_src NULL check first

Quentin Casasnovas
2015-02-24T16:18:28Z

On Tue, Feb 24, 2015 at 11:37:01AM +0100, Borislav Petkov wrote:
> @@ -213,39 +213,46 @@ save_microcode(struct mc_saved_data *mc_saved_data,
> /*
> * Copy new microcode data.
> */
> - mc_saved_p = kmalloc(mc_saved_count*sizeof(struct microcode_intel *),
> + saved_ptr = kmalloc(mc_saved_count * sizeof(struct microcode_intel *),
> GFP_KERNEL);

I'd be tempted to use kcalloc() in these cases but I suppose it's just
personnal preference - it avoids having to make sure your multiplication
cannot overflow when reviewing.

> - if (!mc_saved_p)
> + if (!saved_ptr)
> return -ENOMEM;
>
> for (i = 0; i < mc_saved_count; i++) {
> - struct microcode_intel *mc = mc_saved_src[i];
> - struct microcode_header_intel *mc_header = &mc->hdr;
> - unsigned long mc_size = get_totalsize(mc_header);
> - mc_saved_p[i] = kmalloc(mc_size, GFP_KERNEL);
> - if (!mc_saved_p[i]) {
> - ret = -ENOMEM;
> - goto err;
> - }
> + struct microcode_header_intel *mc_hdr;
> + struct microcode_intel *mc;
> + unsigned long size;
> +
> if (!mc_saved_src[i]) {
> ret = -EINVAL;
> goto err;
> }

... though in this particular case, I think using kcalloc() above would
also prevent the introduction of a kfree() on random junk if
!mc_saved_src[0] since you'll jump straight to err which will
kfree(saved_ptr[0]), which isn't initialized yet. So it might be worth the
change :)

>
> err:
> for (j = 0; j <= i; j++)
> - kfree(mc_saved_p[j]);
> - kfree(mc_saved_p);
> + kfree(saved_ptr[j]);
> + kfree(saved_ptr);
>

Quentin