linux-kernel - Re: [PATCH 02/13] x86/microcode/intel: Do the mc_saved

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 24 Feb 2015 17:20:08 +0100
From:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	Quentin Casasnovas <quentin.casasnovas@...cle.com>
Subject: Re: [PATCH 02/13] x86/microcode/intel: Do the mc_saved_src NULL
 check first

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/