linux-kernel - Re: [PATCH v2 3/4] x86/mce/apei: Handle variable register array size

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: Wed, 26 Jun 2024 12:28:24 -0500
From: "Naik, Avadhut" <avadnaik@....com>
To: Borislav Petkov <bp@...en8.de>, Avadhut Naik <avadhut.naik@....com>
Cc: x86@...nel.org, linux-edac@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
 linux-kernel@...r.kernel.org, tony.luck@...el.com, rafael@...nel.org,
 tglx@...utronix.de, mingo@...hat.com, rostedt@...dmis.org, lenb@...nel.org,
 mchehab@...nel.org, james.morse@....com, airlied@...il.com,
 yazen.ghannam@....com, john.allen@....com
Subject: Re: [PATCH v2 3/4] x86/mce/apei: Handle variable register array size



On 6/26/2024 06:57, Borislav Petkov wrote:
> On Tue, Jun 25, 2024 at 02:56:23PM -0500, Avadhut Naik wrote:
>> From: Yazen Ghannam <yazen.ghannam@....com>
>>
>> ACPI Boot Error Record Table (BERT) is being used by the kernel to
>> report errors that occurred in a previous boot. On some modern AMD
>> systems, these very errors within the BERT are reported through the
>> x86 Common Platform Error Record (CPER) format which consists of one
>> or more Processor Context Information Structures. These context
>> structures provide a starting address and represent an x86 MSR range
>> in which the data constitutes a contiguous set of MSRs starting from,
>> and including the starting address.
>>
>> It's common, for AMD systems that implement this behavior, that the
>> MSR range represents the MCAX register space used for the Scalable MCA
>> feature. The apei_smca_report_x86_error() function decodes and passes
>> this information through the MCE notifier chain. However, this function
>> assumes a fixed register size based on the original HW/FW implementation.
>>
>> This assumption breaks with the addition of two new MCAX registers viz.
>> MCA_SYND1 and MCA_SYND2. These registers are added at the end of the
>> MCAX register space, so they won't be included when decoding the CPER
>> data.
>>
>> Rework apei_smca_report_x86_error() to support a variable register array
>> size. This covers any case where the MSR context information starts at
>> the MCAX address for MCA_STATUS and ends at any other register within
>> the MCAX register space.
>>
>> Add code comments indicating the MCAX register at each offset.
>>
>> [Yazen: Add Avadhut as co-developer for wrapper changes.]
>>
>> Co-developed-by: Avadhut Naik <avadhut.naik@....com>
>> Signed-off-by: Avadhut Naik <avadhut.naik@....com>
>> Signed-off-by: Yazen Ghannam <yazen.ghannam@....com>
> 
> This needs Avadhut's SOB after Yazen's.
> 
Will do. Will change to the below format:

Co-developed-by: Avadhut Naik <avadhut.naik@....com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@....com>
Signed-off-by: Avadhut Naik <avadhut.naik@....com>

> Touchups ontop:
> 
> diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
> index 7a15f0ca1bd1..6bbeb29125a9 100644
> --- a/arch/x86/kernel/cpu/mce/apei.c
> +++ b/arch/x86/kernel/cpu/mce/apei.c
> @@ -69,7 +69,7 @@ EXPORT_SYMBOL_GPL(apei_mce_report_mem_error);
>  int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
>  {
>  	const u64 *i_mce = ((const u64 *) (ctx_info + 1));
> -	unsigned int cpu, num_registers;
> +	unsigned int cpu, num_regs;
>  	struct mce_hw_err err;
>  	struct mce *m = &err.m;
>  
> @@ -93,10 +93,10 @@ int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
>  	/*
>  	 * The number of registers in the register array is determined by
>  	 * Register Array Size/8 as defined in UEFI spec v2.8, sec N.2.4.2.2.
> -	 * Ensure that the array size includes at least 1 register.
> +	 * Sanity-check registers array size.
>  	 */
> -	num_registers = ctx_info->reg_arr_size >> 3;
> -	if (!num_registers)
> +	num_regs = ctx_info->reg_arr_size >> 3;
> +	if (!num_regs)
>  		return -EINVAL;
>  
>  	mce_setup(m);
> @@ -118,13 +118,12 @@ int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
>  	/*
>  	 * The SMCA register layout is fixed and includes 16 registers.
>  	 * The end of the array may be variable, but the beginning is known.
> -	 * Switch on the number of registers. Cap the number of registers to
> -	 * expected max (15).
> +	 * Cap the number of registers to expected max (15).
>  	 */
> -	if (num_registers > 15)
> -		num_registers = 15;
> +	if (num_regs > 15)
> +		num_regs = 15;
>  
> -	switch (num_registers) {
> +	switch (num_regs) {
>  	/* MCA_SYND2 */
>  	case 15:
>  		err.vi.amd.synd2 = *(i_mce + 14);
> 
Will incorporate these.

-- 
Thanks,
Avadhut Naik