netdev - REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey

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: <20220523022438.ofhehjievu2alj3h@lion.mk-sys.cz>
Date:   Mon, 23 May 2022 04:24:38 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jiasheng Jiang <jiasheng@...as.ac.cn>
Cc:     steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: REGRESSION (?) (Re: [PATCH] net: af_key: add check for
 pfkey_broadcast in function pfkey_process)

On Tue, May 17, 2022 at 05:42:31PM +0800, Jiasheng Jiang wrote:
> If skb_clone() returns null pointer, pfkey_broadcast() will
> return error.
> Therefore, it should be better to check the return value of
> pfkey_broadcast() and return error if fails.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jiasheng Jiang <jiasheng@...as.ac.cn>
> ---
>  net/key/af_key.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index fd51db3be91c..92e9d75dba2f 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -2826,8 +2826,10 @@ static int pfkey_process(struct sock *sk, struct sk_buff *skb, const struct sadb
>  	void *ext_hdrs[SADB_EXT_MAX];
>  	int err;
>  
> -	pfkey_broadcast(skb_clone(skb, GFP_KERNEL), GFP_KERNEL,
> -			BROADCAST_PROMISC_ONLY, NULL, sock_net(sk));
> +	err = pfkey_broadcast(skb_clone(skb, GFP_KERNEL), GFP_KERNEL,
> +			      BROADCAST_PROMISC_ONLY, NULL, sock_net(sk));
> +	if (err)
> +		return err;
>  
>  	memset(ext_hdrs, 0, sizeof(ext_hdrs));
>  	err = parse_exthdrs(skb, hdr, ext_hdrs);

After upgrading from 5.18-rc7 to 5.18 final, my racoon daemon refuses to
start because it cannot find some algorithms (it says "aes"). I have not
finished the debugging completely but this patch, mainline commit
4dc2a5a8f675 ("net: af_key: add check for pfkey_broadcast in function
pfkey_process"), seems to be the most promising candidate.

As far as I can see, pfkey_broadcast() returns -ESRCH whenever it does not
send the message to at least one registered listener. But this cannot
happen here even if there were one as BROADCAST_PROMISC_ONLY flag makes
pfkey_broadcast() skip the rest of the loop before err could be set:

	sk_for_each_rcu(sk, &net_pfkey->table) {
...
		if (broadcast_flags != BROADCAST_ALL) {
			if (broadcast_flags & BROADCAST_PROMISC_ONLY)
				continue;
			if ((broadcast_flags & BROADCAST_REGISTERED) &&
			    !pfk->registered)
				continue;
			if (broadcast_flags & BROADCAST_ONE)
				continue;
		}

		err2 = pfkey_broadcast_one(skb, GFP_ATOMIC, sk);

		/* Error is cleared after successful sending to at least one
		 * registered KM */
		if ((broadcast_flags & BROADCAST_REGISTERED) && err)
			err = err2;
	}

and the only other option to change err from -ESRCH is

	if (one_sk != NULL)
		err = pfkey_broadcast_one(skb, allocation, one_sk);

which cannot happen either as one_sk is null when called from
pfkey_process().

So unless I missed something, bailing out on any non-zero return value in
pfkey_process() is wrong without reworking the logic of pfkey_broadcast()
return value first. 

Michal

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)