netdev - Re: [net] tipc: fix using smp_processor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f81eafce-e1d1-bb18-cb70-cfdf45bb2ed0@gmail.com>
Date:   Mon, 31 Aug 2020 10:14:50 +0200
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Tuong Lien <tuong.t.lien@...tech.com.au>, davem@...emloft.net,
        jmaloy@...hat.com, maloy@...jonn.com, ying.xue@...driver.com,
        netdev@...r.kernel.org
Cc:     tipc-discussion@...ts.sourceforge.net
Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible



On 8/29/20 12:37 PM, Tuong Lien wrote:
> The 'this_cpu_ptr()' is used to obtain the AEAD key' TFM on the current
> CPU for encryption, however the execution can be preemptible since it's
> actually user-space context, so the 'using smp_processor_id() in
> preemptible' has been observed.
> 
> We fix the issue by using the 'get/put_cpu_ptr()' API which consists of
> a 'preempt_disable()' instead.
> 
> Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")

Have you forgotten ' Reported-by: syzbot+263f8c0d007dc09b2dda@...kaller.appspotmail.com' ?

> Acked-by: Jon Maloy <jmaloy@...hat.com>
> Signed-off-by: Tuong Lien <tuong.t.lien@...tech.com.au>
> ---
>  net/tipc/crypto.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index c38babaa4e57..7c523dc81575 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -326,7 +326,8 @@ static void tipc_aead_free(struct rcu_head *rp)
>  	if (aead->cloned) {
>  		tipc_aead_put(aead->cloned);
>  	} else {
> -		head = *this_cpu_ptr(aead->tfm_entry);
> +		head = *get_cpu_ptr(aead->tfm_entry);
> +		put_cpu_ptr(aead->tfm_entry);

Why is this safe ?

I think that this very unusual construct needs a comment, because this is not obvious.

This really looks like an attempt to silence syzbot to me.

>  		list_for_each_entry_safe(tfm_entry, tmp, &head->list, list) {
>  			crypto_free_aead(tfm_entry->tfm);
>  			list_del(&tfm_entry->list);
> @@ -399,10 +400,15 @@ static void tipc_aead_users_set(struct tipc_aead __rcu *aead, int val)
>   */
>  static struct crypto_aead *tipc_aead_tfm_next(struct tipc_aead *aead)
>  {
> -	struct tipc_tfm **tfm_entry = this_cpu_ptr(aead->tfm_entry);
> +	struct tipc_tfm **tfm_entry;
> +	struct crypto_aead *tfm;
>  
> +	tfm_entry = get_cpu_ptr(aead->tfm_entry);
>  	*tfm_entry = list_next_entry(*tfm_entry, list);
> -	return (*tfm_entry)->tfm;
> +	tfm = (*tfm_entry)->tfm;
> +	put_cpu_ptr(tfm_entry);

Again, this looks suspicious to me. I can not explain why this would be safe.

> +
> +	return tfm;
>  }
>  
>  /**
>