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: <7f295784-b833-479a-daf4-84e4f89ec694@kernel.org>
Date: Fri, 14 Jul 2023 08:49:55 -0600
From: David Ahern <dsahern@...nel.org>
To: Maciej Żenczykowski <maze@...gle.com>,
 Jiri Pirko <jiri@...nulli.us>
Cc: Linux Network Development Mailing List <netdev@...r.kernel.org>,
 "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] ipv6 addrconf: fix bug where deleting a mngtmpaddr
 can create a new temporary address

On 7/13/23 9:03 AM, Maciej Żenczykowski wrote:
> 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...

I am hoping Jiri can recall why that part was added since it has the
side effect of adding an address on a delete which should not happen.