[PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace

[PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace

[Elliot Berman]

If rcu_read_lock_sched tracing is enabled, the tracing subsystem can
perform a jump which needs to be checked by CFI. For example, stm_ftrace
source is enabled as a module and hooks into enabled ftrace events. This
can cause an recursive loop where find_shadow_check_fn ->
rcu_read_lock_sched -> (call to stm_ftrace generates cfi slowpath) ->
find_shadow_check_fn -> rcu_read_lock_sched -> ...

To avoid the recursion, either the ftrace codes needs to be marked with
__no_cfi or CFI should not trace. Use the "_notrace" in CFI to avoid
tracing so that CFI can guard ftrace.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 kernel/cfi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/cfi.c b/kernel/cfi.c
index e17a56639766..9594cfd1cf2c 100644
--- a/kernel/cfi.c
+++ b/kernel/cfi.c
@@ -248,9 +248,9 @@ static inline cfi_check_fn find_shadow_check_fn(unsigned long ptr)
 {
 	cfi_check_fn fn;
 
-	rcu_read_lock_sched();
+	rcu_read_lock_sched_notrace();
 	fn = ptr_to_check_fn(rcu_dereference_sched(cfi_shadow), ptr);
-	rcu_read_unlock_sched();
+	rcu_read_unlock_sched_notrace();
 
 	return fn;
 }
@@ -269,11 +269,11 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr)
 	cfi_check_fn fn = NULL;
 	struct module *mod;
 
-	rcu_read_lock_sched();
+	rcu_read_lock_sched_notrace();
 	mod = __module_address(ptr);
 	if (mod)
 		fn = mod->cfi_check;
-	rcu_read_unlock_sched();
+	rcu_read_unlock_sched_notrace();
 
 	return fn;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace

[Sami Tolvanen]

Hi Elliot,

On Wed, Aug 11, 2021 at 9:00 AM Elliot Berman <quic_eberman@quicinc.com> wrote:
>
> If rcu_read_lock_sched tracing is enabled, the tracing subsystem can
> perform a jump which needs to be checked by CFI. For example, stm_ftrace
> source is enabled as a module and hooks into enabled ftrace events. This
> can cause an recursive loop where find_shadow_check_fn ->
> rcu_read_lock_sched -> (call to stm_ftrace generates cfi slowpath) ->
> find_shadow_check_fn -> rcu_read_lock_sched -> ...
>
> To avoid the recursion, either the ftrace codes needs to be marked with
> __no_cfi or CFI should not trace. Use the "_notrace" in CFI to avoid
> tracing so that CFI can guard ftrace.
>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  kernel/cfi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index e17a56639766..9594cfd1cf2c 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -248,9 +248,9 @@ static inline cfi_check_fn find_shadow_check_fn(unsigned long ptr)
>  {
>         cfi_check_fn fn;
>
> -       rcu_read_lock_sched();
> +       rcu_read_lock_sched_notrace();
>         fn = ptr_to_check_fn(rcu_dereference_sched(cfi_shadow), ptr);
> -       rcu_read_unlock_sched();
> +       rcu_read_unlock_sched_notrace();
>
>         return fn;
>  }
> @@ -269,11 +269,11 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr)
>         cfi_check_fn fn = NULL;
>         struct module *mod;
>
> -       rcu_read_lock_sched();
> +       rcu_read_lock_sched_notrace();
>         mod = __module_address(ptr);
>         if (mod)
>                 fn = mod->cfi_check;
> -       rcu_read_unlock_sched();
> +       rcu_read_unlock_sched_notrace();
>
>         return fn;
>  }

Thanks for the patch! I agree, this looks like a better solution than
littering tracing code with __nocfi annotations.

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace

[Kees Cook]

On Wed, Aug 11, 2021 at 08:59:14AM -0700, Elliot Berman wrote:
> If rcu_read_lock_sched tracing is enabled, the tracing subsystem can
> perform a jump which needs to be checked by CFI. For example, stm_ftrace
> source is enabled as a module and hooks into enabled ftrace events. This
> can cause an recursive loop where find_shadow_check_fn ->
> rcu_read_lock_sched -> (call to stm_ftrace generates cfi slowpath) ->
> find_shadow_check_fn -> rcu_read_lock_sched -> ...
> 
> To avoid the recursion, either the ftrace codes needs to be marked with
> __no_cfi or CFI should not trace. Use the "_notrace" in CFI to avoid
> tracing so that CFI can guard ftrace.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>

Thanks for this patch! While applying it I noticed that the DKIM
signature failed. This is actually even visible in the lore archive:
https://lore.kernel.org/lkml/20210811155914.19550-1-quic_eberman@quicinc.com/raw
(DKIM_INVALID)

$ b4 am -tls https://lore.kernel.org/lkml/20210811155914.19550-1-quic_eberman@quicinc.com/
Grabbing thread from lore.kernel.org/lkml/20210811155914.19550-1-quic_eberman%40quicinc.com/t.mbox.gz
Analyzing 2 messages in the thread
Checking attestation on all messages, may take a moment...
---
  ✗ [PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace
    + Reviewed-by: Sami Tolvanen <samitolvanen@google.com> (✓ DKIM/google.com)
    + Signed-off-by: Kees Cook <keescook@chromium.org>
    + Link: https://lore.kernel.org/r/20210811155914.19550-1-quic_eberman@quicinc.com
  ---
  ✗ BADSIG: DKIM/quicinc.com



Do you know if qualcomm is mangling outbound emails? (i.e. was the
trailing body suffix added after calculating the DKIM signature?)

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace

[Konstantin Ryabitsev]

On Wed, Aug 11, 2021 at 01:10:36PM -0700, Kees Cook wrote:
> Do you know if qualcomm is mangling outbound emails? (i.e. was the
> trailing body suffix added after calculating the DKIM signature?)

This may not be the case -- the DKIM signature uses "simple/simple"
normalization, meaning that all headers are preserved wrt upper/lowercase and
whitespace. These tend to not survive mailing list managers, unfortunately.

Kees, if you still have the copy that arrived straight at your chromium.org
address (not via vger), you can diff it with what you get from lore to see
where the culprit is.

-K

Re: [PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace

[Elliot Berman]

Hi Kees,

On 8/11/2021 1:10 PM, Kees Cook wrote:
> On Wed, Aug 11, 2021 at 08:59:14AM -0700, Elliot Berman wrote:
>> If rcu_read_lock_sched tracing is enabled, the tracing subsystem can
>> perform a jump which needs to be checked by CFI. For example, stm_ftrace
>> source is enabled as a module and hooks into enabled ftrace events. This
>> can cause an recursive loop where find_shadow_check_fn ->
>> rcu_read_lock_sched -> (call to stm_ftrace generates cfi slowpath) ->
>> find_shadow_check_fn -> rcu_read_lock_sched -> ...
>>
>> To avoid the recursion, either the ftrace codes needs to be marked with
>> __no_cfi or CFI should not trace. Use the "_notrace" in CFI to avoid
>> tracing so that CFI can guard ftrace.
>>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> 
> Thanks for this patch! While applying it I noticed that the DKIM
> signature failed. This is actually even visible in the lore archive:
> https://lore.kernel.org/lkml/20210811155914.19550-1-quic_eberman@quicinc.com/raw
> (DKIM_INVALID)
> 
> $ b4 am -tls https://lore.kernel.org/lkml/20210811155914.19550-1-quic_eberman@quicinc.com/
> Grabbing thread from lore.kernel.org/lkml/20210811155914.19550-1-quic_eberman%40quicinc.com/t.mbox.gz
> Analyzing 2 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
>    ✗ [PATCH 1/1] cfi: Use rcu_read_{un}lock_sched_notrace
>      + Reviewed-by: Sami Tolvanen <samitolvanen@google.com> (✓ DKIM/google.com)
>      + Signed-off-by: Kees Cook <keescook@chromium.org>
>      + Link: https://lore.kernel.org/r/20210811155914.19550-1-quic_eberman@quicinc.com
>    ---
>    ✗ BADSIG: DKIM/quicinc.com
> 
> 
> 
> Do you know if qualcomm is mangling outbound emails? (i.e. was the
> trailing body suffix added after calculating the DKIM signature?)

It's possible. I will check with our IT department. You may be aware 
that Qualcomm was previously using @codeaurora.org mails and this is my 
first time using new mail address.

I tried sending a patch to my personal Gmail address and it looked to be 
happy with the DKIM signature provided.

Please let me know if I should resend the patch a different way for you 
to be able to pull it in.

> 
> Thanks!
> 
> -Kees
>