netdev - Re: [PATCH net-next 1/2] net:dsa: fix dsa_dst_del

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>] [day] [month] [year] [list]

Message-ID: <20170104024553.GC10768@lunn.ch>
Date:   Wed, 4 Jan 2017 03:45:53 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Volodymyr Bendiuga <volodymyr.bendiuga@...termo.se>
Cc:     vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        davem@...emloft.net, netdev@...r.kernel.org,
        volodymyr.bendiuga@...il.com
Subject: Re: [PATCH net-next 1/2] net:dsa: fix dsa_dst_del_ds()

On Wed, Dec 07, 2016 at 05:53:05PM +0100, Volodymyr Bendiuga wrote:
> When dsa_dst_del_ds() is called, do not free the whole list,
> instead, only decrement refcount for the switch tree. The list
> will be deleted in dsa_put_dst() if refcount is 0. Nothing
> really needs to be freed for dsa switch, therefore dsa_free_ds()
> is empty. kref_put() will print warning if dsa_free_ds() is not
> passed as a parameter to it.

This does not look correct. I would expect there to be some symmetry.

The dst gets allocated in _dsa_register_switch(), so it should be
freed somewhere in or under _dsa_unregister_switch(). As you say, it
can be freed from dsa_free_dst(), but that is not called from
_dsa_unregister_switch().

dsa_dst_add_ds() and dsa_dst_del_ds() currently look symmetric.  Add
increments the ref count for the tree, del decrements it. When it
reaches zero, the tree is freed. 

dsa_dst_del_ds() is called from _dsa_unregister_switch(), which gives
us the symmetry with _dsa_register_switch().

What problem are you actually seeing? A double free? A use after free?

Thanks
	Andrew