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 <[email protected]>
---
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
On Tue, May 31, 2022 at 12:33 PM Kees Cook <[email protected]> wrote:
>
> 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 <[email protected]>
> > ---
> > 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: [email protected]
Yes, agreed.
> Also, to improve the commit log, under what situation was this problem
> encountered?
On affected devices, this warning is triggered at boot.
Sami
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 <[email protected]>
> ---
> 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: [email protected]
Also, to improve the commit log, under what situation was this problem
encountered?
--
Kees Cook
On Tue, 31 May 2022 10:59:10 -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
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] cfi: Fix __cfi_slowpath_diag RCU usage with cpuidle
https://git.kernel.org/kees/c/57cd6d157eb4
--
Kees Cook