linux-kernel - Re: [PATCH v2] x86/cpu: Fix x86_match_cpu() to match just X86_VENDOR

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: <20240517144312.GBZkdtAOuJZCvxhFbJ@fat_crate.local>
Date: Fri, 17 May 2024 16:43:12 +0200
From: Borislav Petkov <bp@...en8.de>
To: Tony Luck <tony.luck@...el.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	"Peter Zijlstra (Intel)" <peterz@...radead.org>,
	Uros Bizjak <ubizjak@...il.com>,
	Rick Edgecombe <rick.p.edgecombe@...el.com>,
	Arnd Bergmann <arnd@...db.de>, Mateusz Guzik <mjguzik@...il.com>,
	Thomas Renninger <trenn@...e.de>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH v2] x86/cpu: Fix x86_match_cpu() to match just
 X86_VENDOR_INTEL

On Thu, May 16, 2024 at 09:29:25AM -0700, Tony Luck wrote:
> -#define X86_VENDOR_INTEL	0
>  #define X86_VENDOR_CYRIX	1
>  #define X86_VENDOR_AMD		2
>  #define X86_VENDOR_UMC		3
> +#define X86_VENDOR_INTEL	4
>  #define X86_VENDOR_CENTAUR	5
>  #define X86_VENDOR_TRANSMETA	7
>  #define X86_VENDOR_NSC		8
> -- 

>From my last review:

> But the Intel vendor has been 0 for what, 30 years?

> Are you sure no other code in the tree relies on that? Audited?

But nope, apparently that's too much to ask. :-(

modinfo ./arch/x86/events/intel/intel-uncore.ko
filename:       ./arch/x86/events/intel/intel-uncore.ko
license:        GPL
srcversion:     ECE38449B18DD83223B93FD
alias:          cpu:type:x86,ven0000fam0006mod00B6:feature:*
alias:          cpu:type:x86,ven0000fam0006mod00AF:feature:*
alias:          cpu:type:x86,ven0000fam0006mod00BE:feature:*
			     ^^^^^^^^^

Would everything still work if it said "ven0004" now?

So tglx and I just did some poking and we think the best solution would
be to add a __u16 flags field to struct x86_cpu_id right...

struct x86_cpu_id {
        __u16                      vendor;               /*     0     2 */
        __u16                      family;               /*     2     2 */
        __u16                      model;                /*     4     2 */
        __u16                      steppings;            /*     6     2 */
        __u16                      feature;              /*     8     2 */

        /* XXX 6 bytes hole, try to pack */

<--- HERE

        kernel_ulong_t             driver_data;          /*    16     8 */

        /* size: 24, cachelines: 1, members: 6 */
        /* sum members: 18, holes: 1, sum holes: 6 */
        /* last cacheline: 24 bytes */
};

and the 32-bit version has the same hole:

struct x86_cpu_id {
        __u16                      vendor;               /*     0     2 */
        __u16                      family;               /*     2     2 */
        __u16                      model;                /*     4     2 */
        __u16                      steppings;            /*     6     2 */
        __u16                      feature;              /*     8     2 */

        /* XXX 2 bytes hole, try to pack */

<--- HERE

        kernel_ulong_t             driver_data;          /*    12     4 */

        /* size: 16, cachelines: 1, members: 6 */
        /* sum members: 14, holes: 1, sum holes: 2 */
        /* last cacheline: 16 bytes */
};

And then do:

struct x86_cpu_id {
        __u16 vendor;
        __u16 family;
        __u16 model;
        __u16 steppings;
        __u16 feature;  /* bit index */
	__u16 flags;
        kernel_ulong_t driver_data;
};

#define X86_CPU_ID_FLAG_VENDOR_VALID		BIT(0)

and then have the macros in arch/x86/include/asm/cpu_device_id.h set
that valid flag and then have x86_match_cpu() check it.

Then you don't risk a userspace breakage and that x86_match_cpu() crap
thing is fixed.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette