* [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process
@ 2022-05-17 9:42 Jiasheng Jiang
2022-05-18 8:12 ` Steffen Klassert
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jiasheng Jiang @ 2022-05-17 9:42 UTC (permalink / raw)
To: steffen.klassert, herbert, davem, edumazet, kuba, pabeni
Cc: netdev, linux-kernel, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process
2022-05-17 9:42 [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process Jiasheng Jiang
@ 2022-05-18 8:12 ` Steffen Klassert
2022-05-18 12:00 ` patchwork-bot+netdevbpf
2022-05-23 2:24 ` REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process) Michal Kubecek
2 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2022-05-18 8:12 UTC (permalink / raw)
To: Jiasheng Jiang
Cc: herbert, davem, edumazet, kuba, pabeni, netdev, linux-kernel
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process
2022-05-17 9:42 [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process Jiasheng Jiang
2022-05-18 8:12 ` Steffen Klassert
@ 2022-05-18 12:00 ` patchwork-bot+netdevbpf
2022-05-23 2:24 ` REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process) Michal Kubecek
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-05-18 12:00 UTC (permalink / raw)
To: Jiasheng Jiang
Cc: steffen.klassert, herbert, davem, edumazet, kuba, pabeni, netdev,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process)
2022-05-17 9:42 [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process Jiasheng Jiang
2022-05-18 8:12 ` Steffen Klassert
2022-05-18 12:00 ` patchwork-bot+netdevbpf
@ 2022-05-23 2:24 ` Michal Kubecek
2022-05-23 8:33 ` Michal Kubecek
2 siblings, 1 reply; 6+ messages in thread
From: Michal Kubecek @ 2022-05-23 2:24 UTC (permalink / raw)
To: Jiasheng Jiang
Cc: steffen.klassert, herbert, davem, edumazet, kuba, pabeni, netdev,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process)
2022-05-23 2:24 ` REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process) Michal Kubecek
@ 2022-05-23 8:33 ` Michal Kubecek
2022-05-23 14:32 ` Steffen Klassert
0 siblings, 1 reply; 6+ messages in thread
From: Michal Kubecek @ 2022-05-23 8:33 UTC (permalink / raw)
To: Jiasheng Jiang
Cc: steffen.klassert, herbert, davem, edumazet, kuba, pabeni, netdev,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process)
2022-05-23 8:33 ` Michal Kubecek
@ 2022-05-23 14:32 ` Steffen Klassert
0 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2022-05-23 14:32 UTC (permalink / raw)
To: Michal Kubecek
Cc: Jiasheng Jiang, herbert, davem, edumazet, kuba, pabeni, netdev,
linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-23 14:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 9:42 [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process Jiasheng Jiang
2022-05-18 8:12 ` Steffen Klassert
2022-05-18 12:00 ` patchwork-bot+netdevbpf
2022-05-23 2:24 ` REGRESSION (?) (Re: [PATCH] net: af_key: add check for pfkey_broadcast in function pfkey_process) Michal Kubecek
2022-05-23 8:33 ` Michal Kubecek
2022-05-23 14:32 ` Steffen Klassert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).