netdev - Re: [PATCH net] net: hsr: fix suspicious usage in hsr_node_get

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]

Date:   Thu, 10 Feb 2022 18:12:16 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Juhee Kang <claudiajkang@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        eric.dumazet@...il.com, ennoerlangen@...il.com,
        george.mccollister@...il.com, marco.wenzel@...berle.de,
        xiong.zhenwu@....com.cn,
        syzbot+f0eb4f3876de066b128c@...kaller.appspotmail.com
Subject: Re: [PATCH net] net: hsr: fix suspicious usage in hsr_node_get_first

On Thu, Feb 10, 2022 at 03:49:12PM +0000, Juhee Kang wrote:
> Currently, to dereference hlist_node which is result of hlist_first_rcu(),
> rcu_dereference() is used. But, suspicious RCU warnings occur because
> the caller doesn't acquire RCU. So it was solved by adding rcu_read_lock().
> 
> The kernel test robot reports:
>     [   53.750001][ T3597] =============================
>     [   53.754849][ T3597] WARNING: suspicious RCU usage
>     [   53.759833][ T3597] 5.17.0-rc2-syzkaller-00903-g45230829827b #0 Not tainted
>     [   53.766947][ T3597] -----------------------------
>     [   53.771840][ T3597] net/hsr/hsr_framereg.c:34 suspicious rcu_dereference_check() usage!
>     [   53.780129][ T3597] other info that might help us debug this:
>     [   53.790594][ T3597] rcu_scheduler_active = 2, debug_locks = 1
>     [   53.798896][ T3597] 2 locks held by syz-executor.0/3597:
> 
> Fixes: 4acc45db7115 ("net: hsr: use hlist_head instead of list_head for mac addresses")
> Reported-by: Eric Dumazet <eric.dumazet@...il.com>
> Reported-by: syzbot+f0eb4f3876de066b128c@...kaller.appspotmail.com
> Signed-off-by: Juhee Kang <claudiajkang@...il.com>
> ---
>  net/hsr/hsr_framereg.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index b3c6ffa1894d..92abdf855327 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -31,7 +31,10 @@ struct hsr_node *hsr_node_get_first(struct hlist_head *head)
>  {
>  	struct hlist_node *first;
>  
> +	rcu_read_lock();
>  	first = rcu_dereference(hlist_first_rcu(head));
> +	rcu_read_unlock();

Why wasn't this an issue when when hsr_node_get_first() was just list_first_or_null_rcu()?
Full stack trace please?

I am not familiar with the hsr code base, but I don't need more context
than given to realize that this isn't the proper solution. You aren't
really "fixing" anything if you exit the RCU critical section but still
use "first" afterwards. The driver probably needs some proper accessors
from the writer side, with
rcu_dereference_protected(..., lockdep_is_held(&hsr->list_lock));

> +
>  	if (first)
>  		return hlist_entry(first, struct hsr_node, mac_list);
>  
> -- 
> 2.25.1
>