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 <[email protected]>
---
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 <[email protected]>
---
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:
> `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(-)
>
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
Thank you
--
Gustavo
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:
>> 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