netdev - Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

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: Wed, 8 May 2024 10:40:53 +0100
From: Simon Horman <horms@...nel.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	David Ahern <dsahern@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Vasiliy Kovalev <kovalev@...linux.org>,
	Sabrina Dubroca <sd@...asysnail.net>,
	Guillaume Nault <gnault@...hat.com>
Subject: Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

On Wed, May 08, 2024 at 10:55:02AM +0800, Hangbin Liu wrote:
> The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL
> is not defined. In that case if seg6_hmac_init() fails, the
> genl_unregister_family() isn't called.
> 
> At the same time, add seg6_local_exit() and fix the genl unregister order
> in seg6_exit().

It seems that this fixes two, or perhaps three different problems.
Perhaps we should consider two or three patches?

Also, could you explain the implications of changing the unregister order
in the patch description: it should describe why a change is made.

> Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")

I agree that the current manifestation of the first problem
was introduced. But didn't a very similar problem exist before then?
I suspect the fixes tag should refer to an earlier commit.

> Reported-by: Guillaume Nault <gnault@...hat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>

I think these bugs are pretty good examples of why not
to sprinkle #ifdef inside of functions - it makes the
logic hard to reason with.

So while I agree that a minimal fix, along the lines of this patch, is
suitable for 'net'. Could we consider, as a follow-up, refactoring the code
to remove this #ifdef spaghetti? F.e. by providing dummy implementations
of seg6_iptunnel_init()/seg6_iptunnel_exit() and so on.

-- 
pw-bot: changes-requested