linux-kernel - Re: [PATCH] x86/ioperm: Use atomic64_inc_return() in ksys

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: <77294ea6-480b-456e-8102-6c36ed4b7b96@zytor.com>
Date: Sat, 26 Oct 2024 16:28:04 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Uros Bizjak <ubizjak@...il.com>
Cc: Dave Hansen <dave.hansen@...el.com>, 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/ioperm: Use atomic64_inc_return() in ksys_ioperm()

On 10/25/24 14:01, Uros Bizjak wrote:
>>
>> What does ASM_CALL_CONSTRAINT actually do *in the kernel*, *for x86*? There isn't a redzone in the kernel, and there *can't* be, because asynchronous events can clobber data below the stack pointer at any time.
> 
> The reason for ASM_CALL_CONSTRAINT is explained in arch/x86/include/asm/asm.h:
> 
> --q--
> /*
>   * This output constraint should be used for any inline asm which has a "call"
>   * instruction.  Otherwise the asm may be inserted before the frame pointer
>   * gets set up by the containing function.  If you forget to do this, objtool
>   * may print a "call without frame pointer save/setup" warning.
>   */
> register unsigned long current_stack_pointer asm(_ASM_SP);
> #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
> --/q--
> 
> __alternative_atomic64() macro always uses CALL instruction and one of
> alternatives in __arch_{,try_}cmpxchg64_emu() uses CALL as well, so
> according to the above comment, they all qualify for
> ASM_CALL_CONSTRAINT. This constraint is added to the mentioned macros
> in the proposed series [1].
> 
> [1] https://lore.kernel.org/lkml/20241024180612.162045-1-ubizjak@gmail.com/
>

Ugh. I am not criticizing the usage here, but the construct is 
bleacherous, because it converts what is properly an input constraint 
into an inout constraint, which wouldn't be a big deal except for the 
slight fact that older compilers don't allow asm goto to have output 
constraints.

By any sane definition, the constraint should actually be an input 
constraint on the frame pointer itself; something like:

#define ASM_CALL_CONSTRAINT "r" (__builtin_frame_address(0))

... except that "r" really should be a %rbp constraint, but %rbp doesn't 
seem to have a constraint letter. At least gcc 14.2 seems to do the 
right thing anyway, though: __builtin_frame_address(0) seems to force a 
frame pointer to have been created (even with -fomit-frame-pointer 
specified, and in a leaf function), and the value is always passed in 
%rbp (because why on Earth would it do it differently, when it is 
sitting right there?)

	-hpa