linux-kernel - Re: [PATCH v2 09/11] static_call: Make NULL static calls consistent

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: <dadf372b-9dc2-0c71-2860-1db7c59068b4@csgroup.eu>
Date:   Wed, 22 Mar 2023 15:04:36 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Josh Poimboeuf <jpoimboe@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Mark Rutland <mark.rutland@....com>,
        Jason Baron <jbaron@...mai.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Will McVicker <willmcvicker@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 09/11] static_call: Make NULL static calls consistent



Le 22/03/2023 à 05:00, Josh Poimboeuf a écrit :
> NULL static calls have inconsistent behavior.  With HAVE_STATIC_CALL=y
> they're a NOP, but with HAVE_STATIC_CALL=n they go boom.
> 
> That's guaranteed to cause subtle bugs.  Make the behavior consistent by
> making NULL static calls a NOP with HAVE_STATIC_CALL=n.
> 
> This is probably easier than doing the reverse (making NULL static calls
> panic with HAVE_STATIC_CALL=y).  And it seems to match the current use
> cases better: there are several call sites which rely on the NOP
> behavior, whereas no call sites rely on the crashing behavior.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
> ---

> diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> index b70670a98597..27c095c7fc96 100644
> --- a/arch/x86/kernel/static_call.c
> +++ b/arch/x86/kernel/static_call.c
> @@ -89,7 +89,7 @@ static void __ref __static_call_transform(void *insn, enum insn_type type,
>   
>   	case JCC:
>   		if (!func) {
> -			func = __static_call_return;
> +			func = __static_call_return; //FIXME use __static_call_nop()?
>   			if (cpu_feature_enabled(X86_FEATURE_RETHUNK))
>   				func = x86_return_thunk;
>   		}
> @@ -139,33 +139,35 @@ static void __static_call_validate(u8 *insn, bool tail, bool tramp)
>   	BUG();
>   }
>   
> -static inline enum insn_type __sc_insn(bool null, bool tail)
> +static inline enum insn_type __sc_insn(bool nop, bool tail)
>   {
>   	/*
>   	 * Encode the following table without branches:
>   	 *
> -	 *	tail	null	insn
> +	 *	tail	nop	insn
>   	 *	-----+-------+------
>   	 *	  0  |   0   |  CALL
>   	 *	  0  |   1   |  NOP
>   	 *	  1  |   0   |  JMP
>   	 *	  1  |   1   |  RET
>   	 */
> -	return 2*tail + null;
> +	return 2*tail + nop;
>   }
>   
>   void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
>   {
> +	bool nop = (func == __static_call_nop);
> +

Would be clearer to call it 'is_nop', wouldn't it ?

>   	mutex_lock(&text_mutex);
>   
>   	if (tramp) {
>   		__static_call_validate(tramp, true, true);
> -		__static_call_transform(tramp, __sc_insn(!func, true), func, false);
> +		__static_call_transform(tramp, __sc_insn(nop, true), func, false);
>   	}
>   
>   	if (IS_ENABLED(CONFIG_HAVE_STATIC_CALL_INLINE) && site) {
>   		__static_call_validate(site, tail, false);
> -		__static_call_transform(site, __sc_insn(!func, tail), func, false);
> +		__static_call_transform(site, __sc_insn(nop, tail), func, false);
>   	}
>   
>   	mutex_unlock(&text_mutex);

Christophe