Re: [PATCH v7 3/9] x86/microcode/AMD: Integrate verify_patch_size() into verify_patch()

[Borislav Petkov]

On Tue, Jun 19, 2018 at 08:47:33PM +0200, Maciej S. Szmigiero wrote:
> Integrating verify_patch_size() into verify_patch() allows us to check
> whether the indicated patch size makes sense for its indicated CPU family -
> for all CPU families known to the driver.
> 
> If we spot a patch that is longer than expected for its family we'll
> carefully skip over only the expected length to make sure we don't miss
> good patches in case the indicated patch size was something nonsensically
> huge.
> 
> Signed-off-by: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 131 ++++++++++++----------------
>  1 file changed, 57 insertions(+), 74 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 120778771909..67147e784c0e 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -176,9 +176,6 @@ static bool verify_patch_section(const u8 *buf, size_t buf_size, bool early)
>  	return true;
>  }
>  
> -static unsigned int verify_patch_size(u8 family, u32 patch_size,
> -				      unsigned int size);
> -
>  /*
>   * Check whether there is a valid, non-truncated microcode patch of the
>   * right size for a particular family @family at the beginning of a passed
> @@ -192,7 +189,7 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>  			 unsigned int *crnt_size, bool early)
>  {
>  	const u32 *hdr;
> -	u32 patch_size;
> +	u32 patch_size, max_size;
>  	const struct microcode_header_amd *mc_hdr;
>  	u8 patch_fam;
>  
> @@ -204,48 +201,79 @@ static bool verify_patch(u8 family, const u8 *buf, size_t buf_size,
>  	hdr = (const u32 *)buf;
>  	patch_size = hdr[1];
>  
> -	if (buf_size - SECTION_HDR_SIZE < patch_size) {
> +	if (buf_size - SECTION_HDR_SIZE < sizeof(*mc_hdr)) {
>  		if (!early)
> -			pr_err("Patch of size %u truncated.\n", patch_size);
> +			pr_err("Truncated patch header.\n");
>  
>  		*crnt_size = buf_size;
>  		return false;
>  	}
>  
> +	mc_hdr = (const struct microcode_header_amd *)(buf + SECTION_HDR_SIZE);
> +	patch_fam = 0xf + (mc_hdr->processor_rev_id >> 12);
> +
>  	/*
> -	 * Set a patch length limit of slightly less than U32_MAX to
> -	 * prevent overflowing 32-bit variables holding the whole
> -	 * patch section size.
> +	 * Check whether patch_size isn't something nonsensically huge so
> +	 * we don't skip over good patches by mistake.
>  	 */
> -	if (patch_size > U32_MAX - SECTION_HDR_SIZE) {
> -		if (!early)
> -			pr_err("Patch of size %u too large.\n", patch_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
> +#define F17H_MPB_MAX_SIZE 3200
>  
> -		*crnt_size = SECTION_HDR_SIZE + PATCH_MAX_SIZE;
> -		return false;
> +	switch (patch_fam) {
> +	case 0x10 ... 0x13:

This is not how you integrate a function into another one.

In the first patch, you move the code verbatim to the place where you
want it to be, without adding any other changes whatsoever. The diff
needs to show code movement only.

In follow-on patches you do the other changes you've done and you
explain why.

But no need to resend the whole series - just split this patch as
suggested and send the new ones as a reply to this subthread.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.