netdev - Re: [net PATCH] hsr: fix a NULL pointer dereference in hsr_dev

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: <CAMArcTVUo+fwUGEX1KbsxjfZWQdJCCMboVx_XOe-S0A_UtoZWQ@mail.gmail.com>
Date:   Mon, 2 Dec 2019 00:52:27 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     xiyou.wangcong@...il.com, Netdev <netdev@...r.kernel.org>,
        treeze.taeung@...il.com
Subject: Re: [net PATCH] hsr: fix a NULL pointer dereference in hsr_dev_xmit()

On Sun, 1 Dec 2019 at 03:34, David Miller <davem@...emloft.net> wrote:
>
Hi David,
Thank you for the review!

> From: Taehee Yoo <ap420073@...il.com>
> Date: Sat, 30 Nov 2019 14:24:00 +0000
>
> > @@ -226,9 +226,16 @@ static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> >       struct hsr_priv *hsr = netdev_priv(dev);
> >       struct hsr_port *master;
> >
> > +     rcu_read_lock();
> >       master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
>
> I don't want to distract from your bug fix but I had to audit and learn
> how this hsr->ports stuff works while reading your patch.
>
> hsr->ports has supposedly RCU protection...
>
> But add and delete opertions to the port list only occur by newlink
> netlink operations (the device isn't even visible yet at this point)
> and network device teardown (all packet processing paths will quiesce
> beforehand).
>
> Therefore, the port list never changes from it's effectively static
> configuration made at hsr_dev_finalize() time.
>
> The whole driver very inconsistently accesses the hsr->port list,
> and it all works only because of the above invariant.
>
> So let's not try to fix the RCU protection issues here ok?  That
> should be handled separately, and there are no real problems caused by
> the lack of RCU protection here right now.
>
> Thank you.

Why I thought that rcu_read_lock() is needed in the hsr_dev_xmit() is
that hsr_port_get_hsr() and hsr_forward_skb() should be called under
rcu_read_lock() but I found there is no rcu_read_lock() in the TX datapath.
Below is the test code.

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index ddd9605bad04..8a62474eb1f6 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -349,6 +349,8 @@ void hsr_forward_skb(struct sk_buff *skb, struct
hsr_port *port)
 {
        struct hsr_frame_info frame;

+       printk("[TEST]%s %u rcu_read_lock_held() = %d\n",
+               __func__, __LINE__, rcu_read_lock_held());
        if (skb_mac_header(skb) != skb->data) {
                WARN_ONCE(1, "%s:%d: Malformed frame (port_src %s)\n",
                          __FILE__, __LINE__, port->dev->name);

[12990.180324][    C2] [TEST]hsr_forward_skb 353 rcu_read_lock_held() = 1
[12990.337812][ T1946] [TEST]hsr_forward_skb 353 rcu_read_lock_held() = 0
[12990.341608][    C2] [TEST]hsr_forward_skb 353 rcu_read_lock_held() = 1

$ cat /proc/1946/cmdlind
ping192.168.100.2

we could see there is no rcu_read_lock() in the TX datapath.
So I'm sure that rcu_read_lock() should be added to the hsr_dev_xmit().

If rcu_read_lock() is unnecessary in there, please let me know about
the reason.

Thank you!
Taehee