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]

Date: Thu, 13 Jul 2023 08:59:35 -0600
From: David Ahern <dsahern@...nel.org>
To: Maciej Żenczykowski <maze@...gle.com>,
 Maciej Żenczykowski <zenczykowski@...il.com>
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 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`.

> +
> +	if (create && idev->cnf.use_tempaddr > 0) {
>  		/* When a new public address is created as described
>  		 * in [ADDRCONF], also create a new temporary address.
> -		 * Also create a temporary address if it's enabled but
> -		 * no temporary address currently exists.
>  		 */
>  		read_unlock_bh(&idev->lock);
>  		ipv6_create_tempaddr(ifp, false);