[PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process

[PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process

[Jiasheng Jiang]

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@iscas.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);
-- 
2.25.1

Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process

[Steffen Klassert]

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@iscas.ac.cn>

Applied, thanks!

Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Steffen Klassert <steffen.klassert@secunet.com>:

On Tue, 17 May 2022 17:42:31 +0800 you 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@iscas.ac.cn>
> 
> [...]

Here is the summary with links:
  - net: af_key: add check for pfkey_broadcast in function pfkey_process
    https://git.kernel.org/netdev/net/c/4dc2a5a8f675

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

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@iscas.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On Mon, May 23, 2022 at 04:24:38AM +0200, Michal Kubecek wrote:
> 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.

Tested now, reverting commit 4dc2a5a8f675 ("net: af_key: add check for
pfkey_broadcast in function pfkey_process") seems to fix the issue,
after rebuilding the af_key module with this commit reverted and
reloading it, racoon daemon starts and works and /proc/crypto shows
algrorithms it did not without the revert.

We might get away with changing the test to

	if (err && err != -ESRCH)
		return err;

but I'm not sure if bailing up on failed notification broadcast is
really what we want. Also, most other calling sites of pfkey_broadcast()
do not check the return value either so if we want to add the check, it
should probably be done more consistently. So for now, a revert is IMHO
more appropriate.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Steffen Klassert]

On Mon, May 23, 2022 at 10:33:49AM +0200, Michal Kubecek wrote:
> On Mon, May 23, 2022 at 04:24:38AM +0200, Michal Kubecek wrote:
> > 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.
> 
> Tested now, reverting commit 4dc2a5a8f675 ("net: af_key: add check for
> pfkey_broadcast in function pfkey_process") seems to fix the issue,
> after rebuilding the af_key module with this commit reverted and
> reloading it, racoon daemon starts and works and /proc/crypto shows
> algrorithms it did not without the revert.
> 
> We might get away with changing the test to
> 
> 	if (err && err != -ESRCH)
> 		return err;
> 
> but I'm not sure if bailing up on failed notification broadcast is
> really what we want. Also, most other calling sites of pfkey_broadcast()
> do not check the return value either so if we want to add the check, it
> should probably be done more consistently. So for now, a revert is IMHO
> more appropriate.

Yes, let's just revert it. Maybe we should only accept serious security
bugfixes for the pfkey interface and leave everyting else as it is. Noone
really cares for the pfkey code anymore for more than 10 years. People
should switch to the netlink interface.