[PATCH 0/2] bpf: sockmap, fix uninitialized variable and double-free

[PATCH 0/2] bpf: sockmap, fix uninitialized variable and double-free

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

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

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

[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 <gustavo@embeddedor.com>
---
 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

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

[John Fastabend]

On 05/17/2018 07:08 AM, Gustavo A. R. Silva wrote:
> 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 <gustavo@embeddedor.com>
> ---
>  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.

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>

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

[Gustavo A. R. Silva]

Hi John,

On 05/17/2018 12:27 PM, John Fastabend wrote:
> On 05/17/2018 07:08 AM, Gustavo A. R. Silva wrote:
>> 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 <gustavo@embeddedor.com>
>> ---
>>   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
--
Gustavo

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

[Gustavo A. R. Silva]

Hi John,

On 05/17/2018 12:27 PM, John Fastabend wrote:
> On 05/17/2018 07:08 AM, Gustavo A. R. Silva wrote:
>> 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 <gustavo@embeddedor.com>
>> ---
>>   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

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

[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 <gustavo@embeddedor.com>
---
 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

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

[John Fastabend]

On 05/17/2018 07:11 AM, Gustavo A. R. Silva wrote:
> `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 <gustavo@embeddedor.com>
> ---
>  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)
> 

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>

Re: [PATCH 0/2] bpf: sockmap, fix uninitialized variable and double-free

[Daniel Borkmann]

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]'.

Re: [PATCH 0/2] bpf: sockmap, fix uninitialized variable and double-free

[Gustavo A. R. Silva]

Hi Daniel,

On 05/17/2018 03:51 PM, Daniel Borkmann wrote:
> 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!
> 

Glad to help. :)

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

OK. Will do that.

Thanks
--
Gustavo