linux-kernel - Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch

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]

Message-ID: <d5708abe-40ed-4885-919c-b491f61cceb7@suse.com>
Date: Thu, 14 Nov 2024 15:19:33 +0200
From: Nikolay Borisov <nik.borisov@...e.com>
To: Borislav Petkov <bp@...en8.de>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return
 bool



On 14.11.24 г. 14:58 ч., Borislav Petkov wrote:
> On Fri, Oct 18, 2024 at 06:51:50PM +0300, Nikolay Borisov wrote:
>> The result of that function is in essence boolean, so simplify to return
>> the result of the relevant expression. It also makes it follow the
>> convetion used by __verify_patch_section(). No functional changes.
> 
> convetion used by __verify_patch_section(). No functional changes.
> Unknown word [convetion] in commit message.
> Suggestions: ['convection', 'convention', 'conversion', 'confection', 'conviction', 'connection', 'confession']
> 
> You need a spellchecker. :)
> 
>> Signed-off-by: Nikolay Borisov <nik.borisov@...e.com>
>> ---
>>   arch/x86/kernel/cpu/microcode/amd.c | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
>> index 9986cb85c951..37a428b109a2 100644
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -282,7 +282,7 @@ __verify_patch_section(const u8 *buf, size_t buf_size, u32 *sh_psize)
>>    * exceed the per-family maximum). @sh_psize is the size read from the section
>>    * header.
>>    */
>> -static unsigned int __verify_patch_size(u32 sh_psize, size_t buf_size)
>> +static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>>   {
>>   	u8 family = x86_family(bsp_cpuid_1_eax);
>>   	u32 max_size;
> 
> You missed a spot here for the >= 0x15 families. And I think this is more
> readable and more precise what is supposed to be checked here:
> 
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 8bd79ad63437..0211c62bc4c4 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -289,7 +289,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>   	u32 max_size;
>   
>   	if (family >= 0x15)
> -		return min_t(u32, sh_psize, buf_size);
> +		return sh_psize == min_t(u32, sh_psize, buf_size);

Indee.

>   
>   #define F1XH_MPB_MAX_SIZE 2048
>   #define F14H_MPB_MAX_SIZE 1824
> @@ -306,7 +306,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>   		return 0;
>   	}
>   
> -	return sh_psize <= min_t(u32, buf_size, max_size);
> +	return sh_psize == min_t(u32, buf_size, max_size);

For the older families we have a hard upper bound so we want to ensure 
that the size in the header is strictly <= than buf_size, which in turn 
must be <= max_size .


i.e Is it not valid to have sh_psize < buf_size rather than strictly equal ?

>   }
>   
>   /*
>