netdev - Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr can create a new temporary address

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]

Message-ID: <CANP3RGeR8oKQ=JfRWofb47zt9YF3FRqtemjg63C_Mn4i8R79Dg@mail.gmail.com>
Date: Thu, 13 Jul 2023 17:03:04 +0200
From: Maciej Żenczykowski <maze@...gle.com>
To: David Ahern <dsahern@...nel.org>
Cc: Linux Network Development Mailing List <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>, 
	Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr
 can create a new temporary address

On Thu, Jul 13, 2023 at 4:59 PM David Ahern <dsahern@...nel.org> wrote:
>
> On 7/12/23 7:55 AM, Maciej Żenczykowski wrote:
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index e5213e598a04..94cec2075eee 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -2561,12 +2561,18 @@ static void manage_tempaddrs(struct inet6_dev *idev,
> >                       ipv6_ifa_notify(0, ift);
> >       }
> >
> > -     if ((create || list_empty(&idev->tempaddr_list)) &&
> > -         idev->cnf.use_tempaddr > 0) {
> > +     /* Also create a temporary address if it's enabled but no temporary
> > +      * address currently exists.
> > +      * However, we get called with valid_lft == 0, prefered_lft == 0, create == false
> > +      * as part of cleanup (ie. deleting the mngtmpaddr).
> > +      * We don't want that to result in creating a new temporary ip address.
> > +      */
> > +     if (list_empty(&idev->tempaddr_list) && (valid_lft || prefered_lft))
> > +             create = true;
>
> I am not so sure about this part. manage_tempaddrs has 4 callers --
> autoconf (prefix receive), address add, address modify and address
> delete. Seems like all of them have 'create' set properly when an
> address is wanted in which case maybe the answer here is don't let empty
> address list override `create`.

I did consider that and I couldn't quite convince myself that simply
removing "|| list_empty()" from the if statement is necessarily correct
(thus I went with the more obviously correct change).

Are you convinced dropping the || list_empty would work?
I assume it's there for some reason...