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

[Michal Kubecek]

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@xxxxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: PGP signature