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: <5aa498ab-1ad6-900b-75eb-003ea2d4d8b1@intel.com>
Date:   Tue, 1 Nov 2022 13:28:28 -0700
From:   Sohil Mehta <sohil.mehta@...el.com>
To:     Jithu Joseph <jithu.joseph@...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 10/14] platform/x86/intel/ifs: Add metadata validation

On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> The data portion of IFS test image file contains a meta-data

Can we be consistent with meta-data/metadata usage? Multiple patches 
have this dual usage.

The popular usage in the kernel seems to be "metadata". I would suggest:

s/meta-data/metadata

> structure in addition to test data and hashes.
> 
> Introduce the layout of this meta_data structure and validate
> the sanity of certain fields of the new-image before loading.
> 

s/new-image/new image

> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index be37512535f2..bb43fd65d2d2 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -196,6 +196,7 @@ union ifs_status {
>    * @valid_chunks: number of chunks which could be validated.
>    * @status: it holds simple status pass/fail/untested
>    * @scan_details: opaque scan status code from h/w
> + * @cur_batch: suffix indicating the currently loaded test file

What does "suffix" refer to here? I feel how you derive the current 
batch information shouldn't really matter.

>    */
>   struct ifs_data {
>   	int	integrity_cap_bit;
> @@ -205,6 +206,7 @@ struct ifs_data {
>   	int	valid_chunks;
>   	int	status;
>   	u64	scan_details;
> +	int	cur_batch;
>   };
>   

...

>   #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
>   #define IFS_HEADER_VER	2
> +#define META_TYPE_IFS	1

What namespace does this meta type belong to? Is the expectation here 
that IFS will have different meta types? Or in the generic microcode 
header IFS meta can be found using this type?

I am asking since microcode_intel_find_meta_data() would be eventually 
called from other non-ifs places as well.

Can you please point me to the architecture documentation that describes 
this?


> +static int validate_ifs_metadata(struct device *dev)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	struct meta_data *ifs_meta;
> +	char test_file[64];
> +	int ret = -EINVAL;
> +
> +	snprintf(test_file, sizeof(test_file), "%02x-%02x-%02x-%02x.scan",
> +		 boot_cpu_data.x86, boot_cpu_data.x86_model,
> +		 boot_cpu_data.x86_stepping, ifsd->cur_batch);
> +

There are multiple usages of ifsd->cur_batch in this patch. AFAIU, the 
variable is still uninitialized. Would this validation patch make more 
sense after patch 12? The "cur_batch" terminology is introduced there 
and the initialization happens there as well.

> +
> +	if (ifs_meta->current_image != ifsd->cur_batch) {
> +		dev_warn(dev, "Suffix metadata is not matching with filename %s(0x%02x)\n",

What does "suffix metadata" mean? How about:

Currently loaded filename %s(0x%02x) doesn't match with the information 
in the metadata.

Sohil

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ