linux-kernel - Re: [PATCH 08/14] x86/microcode/intel: Meta-data support in microcode file

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, 1 Nov 2022 11:05:15 -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 08/14] x86/microcode/intel: Meta-data support in microcode
 file



On 11/1/2022 1:51 AM, Sohil Mehta wrote:
> How about?
> 
> x86/microcode/intel: Add metadata support

Will reword as you suggest above

> 
>> +struct metadata_header {
>> +    unsigned int        meta_type;
>> +    unsigned int        meta_blk_size;
>> +};
>> +
>> +struct metadata_intel {
>> +    struct metadata_header    meta_hdr;
>> +    unsigned int        meta_bits[];
>> +};
>> +
> 
> Can we avoid the meta_ prefixes in the struct variables since the struct name already includes meta?

Will do

> 
>>   #define DEFAULT_UCODE_DATASIZE    (2000)
>>   #define MC_HEADER_SIZE        (sizeof(struct microcode_header_intel))
>>   #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
>> @@ -76,6 +89,7 @@ extern int __init save_microcode_in_initrd_intel(void);
>>   void reload_ucode_intel(void);
>>   int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf);
>>   int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver);
>> +struct metadata_header *microcode_intel_find_meta_data(void *ucode, unsigned int meta_type);
> 
> Is there a difference between "ucode" and "mc"? They seem to be used interchangeably all over.
> 
> At least to keep it consistent across the exported functions, should the  parameter be named "mc"?

Will change the parameter to mc

> 
>>   int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>>   {
>> -    unsigned long total_size, data_size, ext_table_size;
>> +    unsigned long total_size, data_size, ext_table_size, total_meta;
>>       struct microcode_header_intel *mc_header = mc;
>>       struct extended_sigtable *ext_header = NULL;
>>       u32 sum, orig_sum, ext_sigcount = 0, i;
>>       struct extended_signature *ext_sig;
>> +    struct metadata_header *meta_header;
>> +    unsigned long meta_size = 0;
>>         total_size = get_totalsize(mc_header);
>>       data_size = get_datasize(mc_header);
>> +    total_meta = mc_header->metasize;
>>         if (data_size + MC_HEADER_SIZE > total_size) {
>>           if (print_err)
>> @@ -245,7 +248,7 @@ int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver)
>>       }
>>         if (!ext_table_size)
>> -        return 0;
>> +        goto check_meta;
>>   
> 
> The code flow in this function seems a bit confusing. Can we avoid the goto and make this a bit cleaner?
> 
> There is already a check for ext_table_size above. Can the extended signature checking be merged with that?

Will modify the flow as below

-       if (!ext_table_size)
-               goto check_meta;
-
+       if (ext_table_size) {
        /*
         * Check extended signature checksum: 0 => valid.
         */
         for( ...) {

                        return -EINVAL;
                }
        }
+       }

>> +
>> +check_meta:
>> +    if (!total_meta)
>> +        return 0;
>> +
>> +    meta_header =  (mc + MC_HEADER_SIZE + data_size) - total_meta;
>> +    while (meta_header->meta_type != META_TYPE_END) {
>> +        meta_size += meta_header->meta_blk_size;
>> +        if (!meta_header->meta_blk_size || meta_size > total_meta) {
>> +            if (print_err) {
>> +                pr_err("Bad value for metadata size, aborting.\n");
>> +                return -EINVAL;
>> +            }
> 
> This seems to be returning an error only when print_err is enabled. Otherwise, it treats as a success.
> 

Thanks for pointing this, will remove the {} following the "if (print_err)"

Jithu