linux-kernel - Re: [PATCH 07/14] x86/microcode/intel: Expose microcode_sanity

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 for Android: free password hash cracker in your pocket

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

Message-ID: <dcdc52a6-c1f1-cbe3-81d4-cc06332f61c2@intel.com>
Date:   Tue, 1 Nov 2022 12:06:35 -0700
From:   "Joseph, Jithu" <jithu.joseph@...el.com>
To:     Sohil Mehta <sohil.mehta@...el.com>, <hdegoede@...hat.com>,
        <markgross@...nel.org>
CC:     <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
        <gregkh@...uxfoundation.org>, <ashok.raj@...el.com>,
        <tony.luck@...el.com>, <linux-kernel@...r.kernel.org>,
        <platform-driver-x86@...r.kernel.org>, <patches@...ts.linux.dev>,
        <ravi.v.shankar@...el.com>, <thiago.macieira@...el.com>,
        <athenas.jimenez.gonzalez@...el.com>
Subject: Re: [PATCH 07/14] x86/microcode/intel: Expose
 microcode_sanity_check()



On 11/1/2022 12:28 AM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> 
>> Refactor header version as a parameter and expose this function.
> 
> Isn't the header version part of the microcode data itself?
> 

Header forms the first 48 bytes in a microcode file followed by contents

> Microcode Format
>    +----------------------+  Base
>    |Header Version        |
>    +----------------------+
>    |Update revision       |
>    +----------------------+
> 
> If so, why the need to pass it as a parameter to sanity_check()?
> 

It is for sanity checking if the contents of the input file matches with the usage scenario.
All microcode files have 1 in the header and IFS test images will use 2. 

Existing usages check if the microcode file has Header version 1. This (along with other sanity checks) is done by the below code 
microcode_intel_sanity_check(data, false, MICROCODE_HEADER_VER)

and IFS usages check whether the file has Header version 2 (along with other sanity checks shared with microcode files)
microcode_intel_sanity_check(data, true, IFS_HEADER_VER))

In other words the sanity check will flag somebody mistakenly passing in a microcode file as IFS test image (and vice-versa)


>>
>> No functional change intended.
> 
> Maybe skip this statement. Apart from adding a parameter to an newly exported function, there is a change in an error print as well.
> 
>> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
>> index 5473b094baee..bc3f33a25d7a 100644
>> --- a/arch/x86/kernel/cpu/microcode/intel.c
>> +++ b/arch/x86/kernel/cpu/microcode/intel.c
>> @@ -37,6 +37,8 @@
>>   #include <asm/setup.h>
>>   #include <asm/msr.h>
>>   +#define MICROCODE_HEADER_VER 1
>> +
> 
> Should this define be in a central location, like microcode_intel.h?
> 

It can be moved there

> You would soon be adding a define for IFS_HEADER_VER. Having them defined together would make it easier to follow.
> 
> Sohil


Jithu