2020-01-19 19:44:56

by Brian Vazquez

Subject: [PATCH v2 bpf-next] bpf: Fix memory leaks in generic update/delete batch ops

generic update/delete batch ops functions were using __bpf_copy_key
without properly freeing the memory. Handle the memory allocation and
copy_from_user separately.

Reported-by: Dan Carpenter <[email protected]>
Fixes: aa2e93b8e58e ("bpf: Add generic support for update and delete batch ops")
Signed-off-by: Brian Vazquez <[email protected]>
Acked-by: Yonghong Song <[email protected]>
---
v1->v2:
- Put Fixes subject in a single line (Yonghong Song)
---
kernel/bpf/syscall.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c26a71460f02f..9a840c57f6df7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1239,12 +1239,15 @@ int generic_map_delete_batch(struct bpf_map *map,
if (!max_count)
return 0;

+ key = kmalloc(map->key_size, GFP_USER | __GFP_NOWARN);
+ if (!key)
+ return -ENOMEM;
+
for (cp = 0; cp < max_count; cp++) {
- key = __bpf_copy_key(keys + cp * map->key_size, map->key_size);
- if (IS_ERR(key)) {
- err = PTR_ERR(key);
+ err = -EFAULT;
+ if (copy_from_user(key, keys + cp * map->key_size,
+ map->key_size))
break;
- }

if (bpf_map_is_dev_bound(map)) {
err = bpf_map_offload_delete_elem(map, key);
@@ -1264,6 +1267,8 @@ int generic_map_delete_batch(struct bpf_map *map,
}
if (copy_to_user(&uattr->batch.count, &cp, sizeof(cp)))
err = -EFAULT;
+
+ kfree(key);
return err;
}

@@ -1294,18 +1299,21 @@ int generic_map_update_batch(struct bpf_map *map,
if (!max_count)
return 0;

+ key = kmalloc(map->key_size, GFP_USER | __GFP_NOWARN);
+ if (!key)
+ return -ENOMEM;
+
value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
- if (!value)
+ if (!value) {
+ kfree(key);
return -ENOMEM;
+ }

for (cp = 0; cp < max_count; cp++) {
- key = __bpf_copy_key(keys + cp * map->key_size, map->key_size);
- if (IS_ERR(key)) {
- err = PTR_ERR(key);
- break;
- }
err = -EFAULT;
- if (copy_from_user(value, values + cp * value_size, value_size))
+ if (copy_from_user(key, keys + cp * map->key_size,
+ map->key_size) ||
+ copy_from_user(value, values + cp * value_size, value_size))
break;

err = bpf_map_update_value(map, f, key, value,
--
2.25.0.341.g760bfbb309-goog

2020-01-20 21:41:00

by Daniel Borkmann

Subject: Re: [PATCH v2 bpf-next] bpf: Fix memory leaks in generic update/delete batch ops

On 1/19/20 8:40 PM, Brian Vazquez wrote:
> generic update/delete batch ops functions were using __bpf_copy_key
> without properly freeing the memory. Handle the memory allocation and
> copy_from_user separately.
>
> Reported-by: Dan Carpenter <[email protected]>
> Fixes: aa2e93b8e58e ("bpf: Add generic support for update and delete batch ops")
> Signed-off-by: Brian Vazquez <[email protected]>
> Acked-by: Yonghong Song <[email protected]>

Applied, thanks!