linux-kernel - Re: [PATCH v2] x86/intel: Fix always false range check in x86

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: <2d30ee37-8069-4443-8a80-5233b3b23f66@intel.com>
Date: Tue, 29 Jul 2025 11:49:47 -0700
From: Sohil Mehta <sohil.mehta@...el.com>
To: Suchit Karunakaran <suchitkarunakaran@...il.com>, <tglx@...utronix.de>,
	<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
	<hpa@...or.com>, <darwi@...utronix.de>, <peterz@...radead.org>,
	<ravi.bangoria@....com>
CC: <skhan@...uxfoundation.org>, <linux-kernel-mentees@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] x86/intel: Fix always false range check in x86_vfm
 model matching

Though the title is correct, it is better to make it more precise.
Something like:

x86/cpu/intel: Fix the constant_tsc model check for Pentium 4s

On 7/28/2025 9:26 PM, Suchit Karunakaran wrote:
> Fix a logic bug in early_init_intel() where a conditional range check:
> (c->x86_vfm >= INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_WILLAMETTE)
> was always false due to (PRESCOTT) being numerically greater than the
> upper bound (WILLAMETTE). This triggers:-Werror=logical-op: 
> logical ‘and’ of mutually exclusive tests is always false

We can focus on the why instead of how this error was found. How about
wording it as below?

The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
wrong. Since INTEL_P4_PRESCOTT is numerically greater than
INTEL_P4_WILLAMETTE, the logic always results in false and never sets
X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.

The error was introduced while replacing the x86_model check with a VFM
one. The original check was as follows:

	if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
		(c->x86 == 0x6 && c->x86_model >= 0x0e))
		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);

Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
Cedarmill (model 6) which is the last model released in Family 15.

> The fix corrects the constant ordering to ensure the range is valid:
> (c->x86_vfm >=  INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_CEDARMILL)
> 

This line is unnecessary. Changes that are easily observable in the diff
below don't need to be described in the commit message.

> Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural
> constant_tsc model checks")
> 

The "Fixes:" line doesn't need to be broken down into multiple lines.
All of it can be on the same line.

For patches being reviewed for upstream, adding a stable Cc (Option 1 in
the stable documentation) is the easiest and preferred way to
automatically trigger submissions for stable.

In this case, include the below Cc: line after the Fixes: line.

Cc: <stable@...r.kernel.org> # v6.15

> Signed-off-by: Suchit Karunakaran <suchitkarunakaran@...il.com>
> 
> Changes since v1:
> - Fix incorrect logic

Patch-to-patch changes should be below the --- line. That way they will
get ignored when the patch is applied to the maintainer's tree.

> ---
>  arch/x86/kernel/cpu/intel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 076eaa41b8c8..6f5bd5dbc249 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -262,7 +262,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	if (c->x86_power & (1 << 8)) {
>  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>  		set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> -	} else if ((c->x86_vfm >= INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_WILLAMETTE) ||
> +	} else if ((c->x86_vfm >=  INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_CEDARMILL) ||
>  		   (c->x86_vfm >= INTEL_CORE_YONAH  && c->x86_vfm <= INTEL_IVYBRIDGE)) {
>  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>  	}