[PATCH 0/2] 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(-)

-- 
2.7.4

`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 <redacted>
---
 kernel/bpf/sockmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 41b41fc..c682669 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c

@@ -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)

-- 
2.7.4

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 <redacted>
---
 kernel/bpf/sockmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index c6de139..41b41fc 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c

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

-- 
2.7.4

On 05/17/2018 07:11 AM, Gustavo A. R. Silva wrote:

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 <john.fastabend@gmail.com>

On 05/17/2018 07:08 AM, Gustavo A. R. Silva wrote:

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 <john.fastabend@gmail.com>

Hi John,

On 05/17/2018 12:27 PM, John Fastabend wrote:

On 05/17/2018 07:08 AM, Gustavo A. R. Silva wrote:

quoted

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 <redacted>
---
  kernel/bpf/sockmap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index c6de139..41b41fc 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c

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

Glad to help. :)

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.

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

Thank you

On 05/17/2018 04:04 PM, Gustavo A. R. Silva wrote:

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(-)

Applied to bpf-next, thanks Gustavo!

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

Hi Daniel,

On 05/17/2018 03:51 PM, Daniel Borkmann wrote:

On 05/17/2018 04:04 PM, Gustavo A. R. Silva wrote:

quoted

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(-)

Applied to bpf-next, thanks Gustavo!

Glad to help. :)

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

OK. Will do that.

Thanks
--
Gustavo