Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 31 May 2022 12:33:16 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Sami Tolvanen <samitolvanen@google.com>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        llvm@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cfi: Fix __cfi_slowpath_diag RCU usage with cpuidle
Message-ID: <202205311231.C26F6E4E@keescook>
References: <20220531175910.890307-1-samitolvanen@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220531175910.890307-1-samitolvanen@google.com>
Precedence: bulk

On Tue, May 31, 2022 at 10:59:10AM -0700, Sami Tolvanen wrote:
> RCU_NONIDLE usage during __cfi_slowpath_diag can result in an invalid
> RCU state in the cpuidle code path:
> 
>   WARNING: CPU: 1 PID: 0 at kernel/rcu/tree.c:613 rcu_eqs_enter+0xe4/0x138
>   ...
>   Call trace:
>     rcu_eqs_enter+0xe4/0x138
>     rcu_idle_enter+0xa8/0x100
>     cpuidle_enter_state+0x154/0x3a8
>     cpuidle_enter+0x3c/0x58
>     do_idle.llvm.6590768638138871020+0x1f4/0x2ec
>     cpu_startup_entry+0x28/0x2c
>     secondary_start_kernel+0x1b8/0x220
>     __secondary_switched+0x94/0x98
> 
> Instead, call rcu_irq_enter/exit to wake up RCU only when needed and
> disable interrupts for the entire CFI shadow/module check when we do.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  kernel/cfi.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index 9594cfd1cf2c..08102d19ec15 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -281,6 +281,8 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr)
>  static inline cfi_check_fn find_check_fn(unsigned long ptr)
>  {
>  	cfi_check_fn fn = NULL;
> +	unsigned long flags;
> +	bool rcu_idle;
>  
>  	if (is_kernel_text(ptr))
>  		return __cfi_check;
> @@ -290,13 +292,21 @@ static inline cfi_check_fn find_check_fn(unsigned long ptr)
>  	 * the shadow and __module_address use RCU, so we need to wake it
>  	 * up if necessary.
>  	 */
> -	RCU_NONIDLE({
> -		if (IS_ENABLED(CONFIG_CFI_CLANG_SHADOW))
> -			fn = find_shadow_check_fn(ptr);
> +	rcu_idle = !rcu_is_watching();
> +	if (rcu_idle) {
> +		local_irq_save(flags);
> +		rcu_irq_enter();
> +	}
> +
> +	if (IS_ENABLED(CONFIG_CFI_CLANG_SHADOW))
> +		fn = find_shadow_check_fn(ptr);
> +	if (!fn)
> +		fn = find_module_check_fn(ptr);
>  
> -		if (!fn)
> -			fn = find_module_check_fn(ptr);
> -	});
> +	if (rcu_idle) {
> +		rcu_irq_exit();
> +		local_irq_restore(flags);
> +	}
>  
>  	return fn;
>  }
> -- 
> 2.36.1.255.ge46751e96f-goog
> 

Thanks for the fix! It looks like these tags could be added, yes?

Fixes: cf68fffb66d6 ("add support for Clang CFI")
Cc: stable@vger.kernel.org

Also, to improve the commit log, under what situation was this problem
encountered?

-- 
Kees Cook