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: <CAM_iQpWYrFx-NbnHpHWmVaf7AoF3Zvi1s6i0Egsf7Ct064X0Xw@mail.gmail.com>
Date:   Sat, 30 Nov 2019 13:04:05 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        treeze.taeung@...il.com
Subject: Re: [net PATCH] hsr: fix a NULL pointer dereference in hsr_dev_xmit()

On Sat, Nov 30, 2019 at 10:35 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
> > Test commands:
> >     ip netns add nst
> >     ip link add v0 type veth peer name v1
> >     ip link add v2 type veth peer name v3
> >     ip link set v1 netns nst
> >     ip link set v3 netns nst
> >     ip link add hsr0 type hsr slave1 v0 slave2 v2
> >     ip a a 192.168.100.1/24 dev hsr0
> >     ip link set v0 up
> >     ip link set v2 up
> >     ip link set hsr0 up
> >     ip netns exec nst ip link add hsr1 type hsr slave1 v1 slave2 v3
> >     ip netns exec nst ip a a 192.168.100.2/24 dev hsr1
> >     ip netns exec nst ip link set v1 up
> >     ip netns exec nst ip link set v3 up
> >     ip netns exec nst ip link set hsr1 up
> >     hping3 192.168.100.2 -2 --flood &
> >     modprobe -rv hsr
>
> Looks like the master port got deleted without respecting RCU
> readers, let me look into it.


It seems hsr_del_port() gets called on module removal path too
and we delete the master port before waiting for RCU readers
there.

Does the following patch help anything? It just moves the list_del_rcu()
after synchronize_rcu() only for master port.

diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index ee561297d8a7..c9638ee00d20 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -174,9 +174,9 @@ void hsr_del_port(struct hsr_port *port)

        hsr = port->hsr;
        master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
-       list_del_rcu(&port->port_list);

        if (port != master) {
+               list_del_rcu(&port->port_list);
                if (master) {
                        netdev_update_features(master->dev);
                        dev_set_mtu(master->dev, hsr_get_max_mtu(hsr));
@@ -193,5 +193,7 @@ void hsr_del_port(struct hsr_port *port)

        if (port != master)
                dev_put(port->dev);
+       else
+               list_del_rcu(&port->port_list);
        kfree(port);
 }