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 <[email protected]>
---
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
Hi Elliot,
On Wed, Aug 11, 2021 at 9:00 AM Elliot Berman <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Sami
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 <[email protected]>
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/[email protected]/raw
(DKIM_INVALID)
$ b4 am -tls https://lore.kernel.org/lkml/[email protected]/
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 <[email protected]> (✓ DKIM/google.com)
+ Signed-off-by: Kees Cook <[email protected]>
+ Link: https://lore.kernel.org/r/[email protected]
---
✗ 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
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
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 <[email protected]>
>
> 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/[email protected]/raw
> (DKIM_INVALID)
>
> $ b4 am -tls https://lore.kernel.org/lkml/[email protected]/
> 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 <[email protected]> (✓ DKIM/google.com)
> + Signed-off-by: Kees Cook <[email protected]>
> + Link: https://lore.kernel.org/r/[email protected]
> ---
> ✗ 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
>