netdev - Re: [PATCH] ipv6/addrconf: fix timing bug in tempaddr regen

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5497d8af4630264418ad91513e7eafeb016e6971.camel@redhat.com>
Date:   Thu, 26 May 2022 09:40:14 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Sam Edwards <cfsworks@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Linux Network Development Mailing List 
        <netdev@...r.kernel.org>
Subject: Re: [PATCH] ipv6/addrconf: fix timing bug in tempaddr regen

On Wed, 2022-05-25 at 14:07 -0600, Sam Edwards wrote:
> Bah, I've had to resend this since it went out as HTML yesterday.
> Sorry about the double mailing everyone!
> 
> On Tue, May 24, 2022 at 3:24 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > I looks like with this change the tmp addresses will never hit the
> > DEPRECATED branch ?!?
> 
> The DEPRECATED branch becomes reachable again once this line is hit:
> ifp->regen_count++;
> ...because it causes this condition in the elseif to evaluate false:
> !ifp->regen_count

That condition looks problematic:


	unsigned long regen_advance = ifp->idev->cnf.regen_max_retry *
                                        ifp->idev->cnf.dad_transmits *
                                        max(NEIGH_VAR(ifp->idev->nd_parms, RETRANS_TIME), HZ/100) / HZ;

	if (age >= ifp->prefered_lft - regen_advance) {

'age', 'ifp->prefered_lft' and 'regen_advance' are unsigned, and it
looks like 'regen_advance' is not constrained to be less then 'ifp-
>prefered_lft'. If that happens the condition will (allways) evaluate
to false, there will be no temporary address regenaration,
'regen_count' will be untouched and the temporary address will never
expire...

... unless I missed something relevant, which is totally possible ;)

Otherwise I think we need to explicitly handle the 'regen_advance >
ifp->prefered_lft' condition possibly with something alike:

	unsigned long regen_advance = ifp->idev->cnf.regen_max_retry * //...
	
	regen_adavance = min(regen_advance, ifp->prefered_lft);
	if (age >= ifp->prefered_lft - regen_advance) { //...

Thanks,

Paolo