Skip to contentSkip to conversation

bpf: sockmap, fix uninitialized variable and double-free

PATCH Applied Gustavo A. R. Silva 3 participants 8 messages May 17, 2018

Subsystems (3)
Networking Network drivers BPF
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

cover bpf: sockmap, fix uninitialized variable and double-free

This patchset aims to fix an uninitialized variable issue and a double-free issue in __sock_map_ctx_update_elem.

Both issues were reported by Coverity.

Thanks.

Gustavo A. R. Silva (2):
bpf: sockmap, fix uninitialized variable
bpf: sockmap, fix double-free

 kernel/bpf/sockmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/2 bpf: sockmap, fix uninitialized variable

There is a potential execution path in which variable err is returned without being properly initialized previously.

Fix this by initializing variable err to 0.

Addresses-Coverity-ID: 1468964 ("Uninitialized scalar variable")

Fixes e5cd3abcb31a ("bpf: sockmap, refactor sockmap routines to work

with hashmap")

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 kernel/bpf/sockmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/bpf/sockmap.c +1 -1
index c6de139..41b41fc 100644
@@ -1713,7 +1713,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
 	struct smap_psock_map_entry *e = NULL;
 	struct smap_psock *psock;
 	bool new = false;
-	int err;
+	int err = 0;
 
 	/* 1. If sock map has BPF programs those will be inherited by the
 	 * sock being added. If the sock is already attached to BPF programs
Signature · 1 line
2.7.4
Acked-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] bpf: sockmap, fix uninitialized variable

Quoted 19 lines · Gustavo A. R. Silva

There is a potential execution path in which variable err is returned without being properly initialized previously.

Fix this by initializing variable err to 0.

Addresses-Coverity-ID: 1468964 ("Uninitialized scalar variable")

Fixes e5cd3abcb31a ("bpf: sockmap, refactor sockmap routines to work

with hashmap")

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 kernel/bpf/sockmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/bpf/sockmap.c +0 -0
index c6de139..41b41fc 100644

kernel/bpf/sockmap.c static int __sock_map_ctx_update_elem(struct bpf_map *map, · line 1719

Quoted 8 lines
kernel/bpf/sockmap.c +1 -1
@@ -1713,7 +1713,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
 	struct smap_psock_map_entry *e = NULL;
 	struct smap_psock *psock;
 	bool new = false;
-	int err;
+	int err = 0;
 
 	/* 1. If sock map has BPF programs those will be inherited by the
 	 * sock being added. If the sock is already attached to BPF programs

Thanks for catching this and the quick fix. The path to hit this case is to add a sock to a map (without a BPF program) where the sock already has been added to another map. I don't have any tests for the case with socks in multiple maps so I'll add some to the selftests so I remember this case.

The alternative fix would be to always 'return 0' at the end of the function, but I think its probably better to init err here like above.

Acked-by John Fastabend <[email protected]>
author
#3 lore raw

Re: [PATCH 1/2] bpf: sockmap, fix uninitialized variable

Hi John,

Quoted 25 lines · John Fastabend
Quoted 24 lines · Gustavo A. R. Silva

There is a potential execution path in which variable err is returned without being properly initialized previously.

Fix this by initializing variable err to 0.

Addresses-Coverity-ID: 1468964 ("Uninitialized scalar variable")

Fixes e5cd3abcb31a ("bpf: sockmap, refactor sockmap routines to work

with hashmap")

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  kernel/bpf/sockmap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
kernel/bpf/sockmap.c +1 -1
index c6de139..41b41fc 100644
@@ -1713,7 +1713,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
  	struct smap_psock_map_entry *e = NULL;
  	struct smap_psock *psock;
  	bool new = false;
-	int err;
+	int err = 0;
Quoted 10 lines

kernel/bpf/sockmap.c static int __sock_map_ctx_update_elem(struct bpf_map *map, · line 1719

Quoted 3 lines
kernel/bpf/sockmap.c +0 -0
  	/* 1. If sock map has BPF programs those will be inherited by the
  	 * sock being added. If the sock is already attached to BPF programs

Thanks for catching this and the quick fix. The path to hit this case is to add a sock to a map (without a BPF program) where the sock already has been added to another map. I don't have any tests for the case with socks in multiple maps so I'll add some to the selftests so I remember this case.

Glad to help. :)

Quoted 3 lines

The alternative fix would be to always 'return 0' at the end of the function, but I think its probably better to init err here like above.

Yeah. I think initializing err is better in this case.

Quoted 2 lines
Acked-by John Fastabend <[email protected]>

Thank you

author
#4 lore raw

PATCH 2/2 bpf: sockmap, fix double-free

In reply to Gustavo A. R. Silva

`e' is being freed twice.

Fix this by removing one of the kfree() calls.

Addresses-Coverity-ID: 1468983 ("Double free")

Fixes 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 kernel/bpf/sockmap.c | 1 -
 1 file changed, 1 deletion(-)
kernel/bpf/sockmap.c +0 -1
index 41b41fc..c682669 100644
@@ -1823,7 +1823,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
 	write_unlock_bh(&sock->sk_callback_lock);
 	return err;
 out_free:
-	kfree(e);
 	smap_release_sock(psock, sock);
 out_progs:
 	if (verdict)
Signature · 1 line
2.7.4
Acked-by on v1 2/2
#5 lore raw

Re: [PATCH 2/2] bpf: sockmap, fix double-free

Quoted 17 lines · Gustavo A. R. Silva

`e' is being freed twice.

Fix this by removing one of the kfree() calls.

Addresses-Coverity-ID: 1468983 ("Double free")

Fixes 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 kernel/bpf/sockmap.c | 1 -
 1 file changed, 1 deletion(-)
kernel/bpf/sockmap.c +0 -0
index 41b41fc..c682669 100644

kernel/bpf/sockmap.c static int __sock_map_ctx_update_elem(struct bpf_map *map, · line 1828

Quoted 7 lines
kernel/bpf/sockmap.c +0 -1
@@ -1823,7 +1823,6 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
 	write_unlock_bh(&sock->sk_callback_lock);
 	return err;
 out_free:
-	kfree(e);
 	smap_release_sock(psock, sock);
 out_progs:
 	if (verdict)

Thanks. This can happen when a user tries to add a kTLS socket to a sockmap. We need to add some tests for kTLS + sockmap cases.

Acked-by John Fastabend <[email protected]>
maintainer Applied
#6 lore raw
In reply to Gustavo A. R. Silva
Quoted 5 lines · Gustavo A. R. Silva

This patchset aims to fix an uninitialized variable issue and a double-free issue in __sock_map_ctx_update_elem.

Both issues were reported by Coverity.

Quoted 8 lines

Thanks.

Gustavo A. R. Silva (2):
bpf: sockmap, fix uninitialized variable
bpf: sockmap, fix double-free

 kernel/bpf/sockmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Applied to bpf-next, thanks Gustavo!

P.s.: Please indicate that next time in the email subject via '[PATCH bpf-next]'.

author
#7 lore raw

Hi Daniel,

Quoted 8 lines · Daniel Borkmann
Quoted 7 lines · Gustavo A. R. Silva

This patchset aims to fix an uninitialized variable issue and a double-free issue in __sock_map_ctx_update_elem.

Both issues were reported by Coverity.

Thanks.

Quoted 9 lines
Quoted 6 lines

Gustavo A. R. Silva (2):
bpf: sockmap, fix uninitialized variable
bpf: sockmap, fix double-free

  kernel/bpf/sockmap.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

Applied to bpf-next, thanks Gustavo!

Glad to help. :)

Quoted 2 lines

P.s.: Please indicate that next time in the email subject via '[PATCH bpf-next]'.

OK. Will do that.

Thanks
--
Gustavo