netdev - Re: [PATCH net] net: dsa: felix: suppress non-changes to the tagging protocol

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, 8 Aug 2022 13:49:42 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        UNGLinuxDriver@...rochip.com,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] net: dsa: felix: suppress non-changes to the tagging
 protocol

On 8/8/22 05:51, Vladimir Oltean wrote:
> The way in which dsa_tree_change_tag_proto() works is that when
> dsa_tree_notify() fails, it doesn't know whether the operation failed
> mid way in a multi-switch tree, or it failed for a single-switch tree.
> So even though drivers need to fail cleanly in
> ds->ops->change_tag_protocol(), DSA will still call dsa_tree_notify()
> again, to restore the old tag protocol for potential switches in the
> tree where the change did succeeed (before failing for others).
> 
> This means for the felix driver that if we report an error in
> felix_change_tag_protocol(), we'll get another call where proto_ops ==
> old_proto_ops. If we proceed to act upon that, we may do unexpected
> things. For example, we will call dsa_tag_8021q_register() twice in a
> row, without any dsa_tag_8021q_unregister() in between. Then we will
> actually call dsa_tag_8021q_unregister() via old_proto_ops->teardown,
> which (if it manages to run at all, after walking through corrupted data
> structures) will leave the ports inoperational anyway.
> 
> The bug can be readily reproduced if we force an error while in
> tag_8021q mode; this crashes the kernel.
> 
> echo ocelot-8021q > /sys/class/net/eno2/dsa/tagging
> echo edsa > /sys/class/net/eno2/dsa/tagging # -EPROTONOSUPPORT
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000014
> Call trace:
>   vcap_entry_get+0x24/0x124
>   ocelot_vcap_filter_del+0x198/0x270
>   felix_tag_8021q_vlan_del+0xd4/0x21c
>   dsa_switch_tag_8021q_vlan_del+0x168/0x2cc
>   dsa_switch_event+0x68/0x1170
>   dsa_tree_notify+0x14/0x34
>   dsa_port_tag_8021q_vlan_del+0x84/0x110
>   dsa_tag_8021q_unregister+0x15c/0x1c0
>   felix_tag_8021q_teardown+0x16c/0x180
>   felix_change_tag_protocol+0x1bc/0x230
>   dsa_switch_event+0x14c/0x1170
>   dsa_tree_change_tag_proto+0x118/0x1c0
> 
> Fixes: 7a29d220f4c0 ("net: dsa: felix: reimplement tagging protocol change with function pointers")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian