linux-kernel - Re: [PATCH] x86/boot: Do not test if AC and ID eflags are changeable on 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: <E3E112F8-CC41-4933-9FEC-B53D6A0AFA7A@zytor.com>
Date: Fri, 07 Mar 2025 05:13:26 -0800
From: "H. Peter Anvin" <hpa@...or.com>
To: Uros Bizjak <ubizjak@...il.com>
CC: x86@...nel.org, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...nel.org>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH] x86/boot: Do not test if AC and ID eflags are changeable on x86_64

On March 7, 2025 4:15:42 AM PST, Uros Bizjak <ubizjak@...il.com> wrote:
>On Fri, Mar 7, 2025 at 12:58 PM H. Peter Anvin <hpa@...or.com> wrote:
>>
>> On March 7, 2025 1:10:03 AM PST, Uros Bizjak <ubizjak@...il.com> wrote:
>> >The test for the changeabitily of AC and ID eflags is used to
>> >distinguish between i386 and i486 processors (AC) and to test
>> >for cpuid instruction support (ID).
>> >
>> >Skip these tests on x86_64 processors as they always supports cpuid.
>> >
>> >Also change the return type of has_eflag() to bool.
>> >
>> >Signed-off-by: Uros Bizjak <ubizjak@...il.com>
>> >Cc: Thomas Gleixner <tglx@...utronix.de>
>> >Cc: Ingo Molnar <mingo@...nel.org>
>> >Cc: Borislav Petkov <bp@...en8.de>
>> >Cc: Dave Hansen <dave.hansen@...ux.intel.com>
>> >Cc: "H. Peter Anvin" <hpa@...or.com>
>> >---
>> > arch/x86/boot/cpuflags.c | 26 +++++++++-----------------
>> > arch/x86/boot/cpuflags.h |  6 +++++-
>> > 2 files changed, 14 insertions(+), 18 deletions(-)
>> >
>> >diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c
>> >index d75237ba7ce9..2150a016176f 100644
>> >--- a/arch/x86/boot/cpuflags.c
>> >+++ b/arch/x86/boot/cpuflags.c
>> >@@ -29,40 +29,32 @@ static int has_fpu(void)
>> >       return fsw == 0 && (fcw & 0x103f) == 0x003f;
>> > }
>> >
>> >+#ifdef CONFIG_X86_32
>> > /*
>> >  * For building the 16-bit code we want to explicitly specify 32-bit
>> >  * push/pop operations, rather than just saying 'pushf' or 'popf' and
>> >- * letting the compiler choose. But this is also included from the
>> >- * compressed/ directory where it may be 64-bit code, and thus needs
>> >- * to be 'pushfq' or 'popfq' in that case.
>> >+ * letting the compiler choose.
>> >  */
>> >-#ifdef __x86_64__
>> >-#define PUSHF "pushfq"
>> >-#define POPF "popfq"
>> >-#else
>> >-#define PUSHF "pushfl"
>> >-#define POPF "popfl"
>> >-#endif
>> >-
>> >-int has_eflag(unsigned long mask)
>> >+bool has_eflag(unsigned long mask)
>> > {
>> >       unsigned long f0, f1;
>> >
>> >-      asm volatile(PUSHF "    \n\t"
>> >-                   PUSHF "    \n\t"
>> >+      asm volatile("pushfl    \n\t"
>> >+                   "pushfl    \n\t"
>> >                    "pop %0    \n\t"
>> >                    "mov %0,%1 \n\t"
>> >                    "xor %2,%1 \n\t"
>> >                    "push %1   \n\t"
>> >-                   POPF "     \n\t"
>> >-                   PUSHF "    \n\t"
>> >+                   "popfl     \n\t"
>> >+                   "pushfl    \n\t"
>> >                    "pop %1    \n\t"
>> >-                   POPF
>> >+                   "popfl"
>> >                    : "=&r" (f0), "=&r" (f1)
>> >                    : "ri" (mask));
>> >
>> >       return !!((f0^f1) & mask);
>> > }
>> >+#endif
>> >
>> > void cpuid_count(u32 id, u32 count, u32 *a, u32 *b, u32 *c, u32 *d)
>> > {
>> >diff --git a/arch/x86/boot/cpuflags.h b/arch/x86/boot/cpuflags.h
>> >index fdcc2aa4c3c4..a398d9204ad0 100644
>> >--- a/arch/x86/boot/cpuflags.h
>> >+++ b/arch/x86/boot/cpuflags.h
>> >@@ -15,7 +15,11 @@ struct cpu_features {
>> > extern struct cpu_features cpu;
>> > extern u32 cpu_vendor[3];
>> >
>> >-int has_eflag(unsigned long mask);
>> >+#ifdef CONFIG_X86_32
>> >+bool has_eflag(unsigned long mask);
>> >+#else
>> >+static inline bool has_eflag(unsigned long mask) { return true; }
>> >+#endif
>> > void get_cpuflags(void);
>> > void cpuid_count(u32 id, u32 count, u32 *a, u32 *b, u32 *c, u32 *d);
>> > bool has_cpuflag(int flag);
>>
>> PUSF et al → pushf
>>
>> The -l and -q suffixes have been optional for a long time.
>
>No, not in this case. Please see the comment:
>
>/*
>* For building the 16-bit code we want to explicitly specify 32-bit
>* push/pop operations, rather than just saying 'pushf' or 'popf' and
>* letting the compiler choose.
>*/
>
>We are building 16-bit code here, and we want PUSHFL, the one with
>operand size prefix 0x66.
>
>Please consider the following code:
>
>    .code16
>    pushf
>    pushfl
>
>as -o push.o push.s
>
>objdump -dr -Mdata16 push.o
>
>0000000000000000 <.text>:
>  0:   9c                      pushf
>  1:   66 9c                   pushfl
>
>Uros.
>

*plonk* I should have remembered (.code16gcc is different then .code16 though.) I wrote the damned things after all...