BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer
@ 2020-10-12 17:09 Alex Dewar
  2020-10-14  9:20 ` Jakub Sitnicki
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alex Dewar @ 2020-10-12 17:09 UTC (permalink / raw)
  Cc: Alex Dewar, John Fastabend, Daniel Borkmann, Jakub Sitnicki,
	Lorenz Bauer, David S. Miller, Jakub Kicinski,
	Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
	Andrii Nakryiko, KP Singh, netdev, bpf, linux-kernel

If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
called unconditionally on skb_verdict, even though it may be NULL. Fix
and tidy up error path.

Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
 net/core/sock_map.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index df09c39a4dd2..a73ccce54423 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -238,17 +238,18 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
 	int ret;
 
 	skb_verdict = READ_ONCE(progs->skb_verdict);
-	skb_parser = READ_ONCE(progs->skb_parser);
 	if (skb_verdict) {
 		skb_verdict = bpf_prog_inc_not_zero(skb_verdict);
 		if (IS_ERR(skb_verdict))
 			return PTR_ERR(skb_verdict);
 	}
+
+	skb_parser = READ_ONCE(progs->skb_parser);
 	if (skb_parser) {
 		skb_parser = bpf_prog_inc_not_zero(skb_parser);
 		if (IS_ERR(skb_parser)) {
-			bpf_prog_put(skb_verdict);
-			return PTR_ERR(skb_parser);
+			ret = PTR_ERR(skb_parser);
+			goto out_put_skb_verdict;
 		}
 	}
 
@@ -257,7 +258,7 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
 		msg_parser = bpf_prog_inc_not_zero(msg_parser);
 		if (IS_ERR(msg_parser)) {
 			ret = PTR_ERR(msg_parser);
-			goto out;
+			goto out_put_skb_parser;
 		}
 	}
 
@@ -311,11 +312,12 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
 out_progs:
 	if (msg_parser)
 		bpf_prog_put(msg_parser);
-out:
-	if (skb_verdict)
-		bpf_prog_put(skb_verdict);
+out_put_skb_parser:
 	if (skb_parser)
 		bpf_prog_put(skb_parser);
+out_put_skb_verdict:
+	if (skb_verdict)
+		bpf_prog_put(skb_verdict);
 	return ret;
 }
 
-- 
2.28.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer
  2020-10-12 17:09 [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer Alex Dewar
@ 2020-10-14  9:20 ` Jakub Sitnicki
  2020-10-15  4:43   ` John Fastabend
  2020-10-14  9:32 ` Jakub Sitnicki
  2020-10-15 19:10 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 7+ messages in thread
From: Jakub Sitnicki @ 2020-10-14  9:20 UTC (permalink / raw)
  To: Alex Dewar
  Cc: John Fastabend, Daniel Borkmann, Lorenz Bauer, David S. Miller,
	Jakub Kicinski, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, KP Singh, netdev, bpf,
	linux-kernel

On Mon, Oct 12, 2020 at 07:09 PM CEST, Alex Dewar wrote:
> If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
> called unconditionally on skb_verdict, even though it may be NULL. Fix
> and tidy up error path.
>
> Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
> Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---

Acked-by: Jakub Sitnicki <jakub@cloudflare.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer
  2020-10-12 17:09 [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer Alex Dewar
  2020-10-14  9:20 ` Jakub Sitnicki
@ 2020-10-14  9:32 ` Jakub Sitnicki
  2020-10-14  9:45   ` Alex Dewar
  2020-10-15 19:10 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 7+ messages in thread
From: Jakub Sitnicki @ 2020-10-14  9:32 UTC (permalink / raw)
  To: Daniel Borkmann, David S. Miller, Jakub Kicinski, Alexei Starovoitov
  Cc: Alex Dewar, John Fastabend, Lorenz Bauer, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrii Nakryiko, KP Singh, netdev, bpf,
	linux-kernel

On Mon, Oct 12, 2020 at 07:09 PM CEST, Alex Dewar wrote:
> If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
> called unconditionally on skb_verdict, even though it may be NULL. Fix
> and tidy up error path.
>
> Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
> Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---

Note to maintainers: the issue exists only in bpf-next where we have:

  https://lore.kernel.org/bpf/160239294756.8495.5796595770890272219.stgit@john-Precision-5820-Tower/

The patch also looks like it is supposed to be applied on top of the above.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer
  2020-10-14  9:32 ` Jakub Sitnicki
@ 2020-10-14  9:45   ` Alex Dewar
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Dewar @ 2020-10-14  9:45 UTC (permalink / raw)
  To: Jakub Sitnicki, Daniel Borkmann, David S. Miller, Jakub Kicinski,
	Alexei Starovoitov
  Cc: John Fastabend, Lorenz Bauer, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, KP Singh, netdev, bpf,
	linux-kernel

On 14/10/2020 10:32, Jakub Sitnicki wrote:
> On Mon, Oct 12, 2020 at 07:09 PM CEST, Alex Dewar wrote:
>> If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
>> called unconditionally on skb_verdict, even though it may be NULL. Fix
>> and tidy up error path.
>>
>> Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
>> Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
>> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
>> ---
> Note to maintainers: the issue exists only in bpf-next where we have:
>
>    https://lore.kernel.org/bpf/160239294756.8495.5796595770890272219.stgit@john-Precision-5820-Tower/
>
> The patch also looks like it is supposed to be applied on top of the above.
Yes, the patch is based on linux-next.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer
  2020-10-14  9:20 ` Jakub Sitnicki
@ 2020-10-15  4:43   ` John Fastabend
  2020-10-15 11:04     ` Jakub Sitnicki
  0 siblings, 1 reply; 7+ messages in thread
From: John Fastabend @ 2020-10-15  4:43 UTC (permalink / raw)
  To: Jakub Sitnicki, Alex Dewar
  Cc: John Fastabend, Daniel Borkmann, Lorenz Bauer, David S. Miller,
	Jakub Kicinski, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, KP Singh, netdev, bpf,
	linux-kernel

Jakub Sitnicki wrote:
> On Mon, Oct 12, 2020 at 07:09 PM CEST, Alex Dewar wrote:
> > If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
> > called unconditionally on skb_verdict, even though it may be NULL. Fix
> > and tidy up error path.
> >
> > Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
> > Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
> > Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> > ---
> 
> Acked-by: Jakub Sitnicki <jakub@cloudflare.com>

Thanks.

Jakub, any opinions on if we should just throw an error if users try to
add a sock to a map with a parser but no verdict? At the moment we fall
through and add the socket, but it wont do any receive parsing/verdict.
At the moment I think its fine with above fix. The useful cases for RX
are parser+verdict, verdict, and empty. Where empty is just used for
redirects or other socket account tricks. Just something to keep in mind.

Acked-by: John Fastabend <john.fastabend@gmail.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer
  2020-10-15  4:43   ` John Fastabend
@ 2020-10-15 11:04     ` Jakub Sitnicki
  0 siblings, 0 replies; 7+ messages in thread
From: Jakub Sitnicki @ 2020-10-15 11:04 UTC (permalink / raw)
  To: John Fastabend
  Cc: Alex Dewar, Daniel Borkmann, Lorenz Bauer, David S. Miller,
	Jakub Kicinski, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, KP Singh, netdev, bpf,
	linux-kernel

On Thu, Oct 15, 2020 at 06:43 AM CEST, John Fastabend wrote:

[...]

> Jakub, any opinions on if we should just throw an error if users try to
> add a sock to a map with a parser but no verdict? At the moment we fall
> through and add the socket, but it wont do any receive parsing/verdict.
> At the moment I think its fine with above fix. The useful cases for RX
> are parser+verdict, verdict, and empty. Where empty is just used for
> redirects or other socket account tricks. Just something to keep in mind.

IMO we should not fail because map updates can interleave with sk_skb
prog attachments, like so:

	update_map(map_fd, sock_fd);
	attach_prog(parser_fd, map_fd, BPF_SK_SKB_STREAM_PARSER);
	update_map(map_fd, sock_fd); // OK
	attach_prog(verdict_fd, map_fd, BPF_SK_SKB_STREAM_VERDICT);
	update_map(map_fd, sock_fd);

In practice, I would expect one process/thread to attach the programs,
while another is allowed to update the map at the same time.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer
  2020-10-12 17:09 [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer Alex Dewar
  2020-10-14  9:20 ` Jakub Sitnicki
  2020-10-14  9:32 ` Jakub Sitnicki
@ 2020-10-15 19:10 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2020-10-15 19:10 UTC (permalink / raw)
  To: Alex Dewar
  Cc: john.fastabend, daniel, jakub, lmb, davem, kuba, ast, kafai,
	songliubraving, yhs, andrii, kpsingh, netdev, bpf, linux-kernel

Hello:

This patch was applied to bpf/bpf-next.git (refs/heads/master):

On Mon, 12 Oct 2020 18:09:53 +0100 you wrote:
> If bpf_prog_inc_not_zero() fails for skb_parser, then bpf_prog_put() is
> called unconditionally on skb_verdict, even though it may be NULL. Fix
> and tidy up error path.
> 
> Addresses-Coverity-ID: 1497799: Null pointer dereferences (FORWARD_NULL)
> Fixes: 743df8b7749f ("bpf, sockmap: Check skb_verdict and skb_parser programs explicitly")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> 
> [...]

Here is the summary with links:
  - net: sockmap: Don't call bpf_prog_put() on NULL pointer
    https://git.kernel.org/bpf/bpf-next/c/83c11c17553c

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] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-12 17:09 [PATCH] net: sockmap: Don't call bpf_prog_put() on NULL pointer Alex Dewar
2020-10-14  9:20 ` Jakub Sitnicki
2020-10-15  4:43   ` John Fastabend
2020-10-15 11:04     ` Jakub Sitnicki
2020-10-14  9:32 ` Jakub Sitnicki
2020-10-14  9:45   ` Alex Dewar
2020-10-15 19:10 ` patchwork-bot+netdevbpf

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git