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]

Date:   Mon, 2 Dec 2019 01:08:38 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Cong Wang <xiyou.wangcong@...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 Sun, 1 Dec 2019 at 06:04, Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> 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.

The path is this.
hsr_exit() -> unregister_netdevice_notifier() -> hsr_netdev_notify()
 -> hsr_del_port().

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

Thank you so much for providing the testing code.
I have tested this patch, but I could still see the same panic.

> 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);
>  }