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

From: Michal Kubecek
Date: Sun May 22 2022 - 22:30:17 EST


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