Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 9 Nov 2021 19:49:03 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Jason Baron <jbaron@akamai.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Kees Cook <keescook@chromium.org>
Subject: Re: [RFC PATCH 2/7] static_call: deal with unexported keys without
 cluttering up the API
Message-ID: <YYrCn3mc+EbY+OB/@hirez.programming.kicks-ass.net>
References: <20211109164549.1724710-1-ardb@kernel.org>
 <20211109164549.1724710-3-ardb@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211109164549.1724710-3-ardb@kernel.org>
Precedence: bulk

On Tue, Nov 09, 2021 at 05:45:44PM +0100, Ard Biesheuvel wrote:

> diff --git a/include/linux/static_call_types.h b/include/linux/static_call_types.h
> index 5a00b8b2cf9f..0bb36294cce7 100644
> --- a/include/linux/static_call_types.h
> +++ b/include/linux/static_call_types.h
> @@ -32,15 +32,20 @@
>  struct static_call_site {
>  	s32 addr;
>  	s32 key;
> +	s32 tramp;
>  };

I can't say I'm thrilled at growing this thing, but the cleanup is nice.
Perhaps we can increase alignment on struct static_call_key and instead
frob it in .key still?

>  
>  #define DECLARE_STATIC_CALL(name, func)					\
> -	extern struct static_call_key STATIC_CALL_KEY(name);		\
> +	extern __weak struct static_call_key STATIC_CALL_KEY(name);	\
>  	extern typeof(func) STATIC_CALL_TRAMP(name);

I'm a little bit confused on how this actually works. What does a __weak
extern data symbol do?

A __weak function definition would create a module local instance of the
function barring a strong override.

But what does a __weak extern do?

> diff --git a/kernel/static_call.c b/kernel/static_call.c
> index 43ba0b1e0edb..360cc3cd0fbf 100644
> --- a/kernel/static_call.c
> +++ b/kernel/static_call.c
> @@ -366,18 +366,18 @@ static int static_call_add_module(struct module *mod)
>  		 * means modules are allowed to call static_call_update() on
>  		 * it.
>  		 *
> -		 * Otherwise, the key isn't exported, and 'addr' points to the
> +		 * Otherwise, the key isn't exported, and 'tramp' points to the
>  		 * trampoline so we need to lookup the key.
>  		 *
>  		 * We go through this dance to prevent crazy modules from
>  		 * abusing sensitive static calls.
>  		 */
> -		if (!kernel_text_address(addr))
> +		if (addr)
>  			continue;

This seems to imply that the __weak extern symbol gets resolved to 0 at
module link time.

>  
> -		key = tramp_key_lookup(addr);
> +		key = tramp_key_lookup((unsigned long)offset_to_ptr(&site->tramp));
>  		if (!key) {
> -			pr_warn("Failed to fixup __raw_static_call() usage at: %ps\n",
> +			pr_warn("Failed to fixup static_call() usage at: %ps\n",
>  				static_call_addr(site));
>  			return -EINVAL;

>  		}