On Fri, Jan 17, 2020 at 2:44 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Brian Vazquez,
>
> The patch aa2e93b8e58e: "bpf: Add generic support for update and
> delete batch ops" from Jan 15, 2020, leads to the following static
> checker warning:
>
> kernel/bpf/syscall.c:1322 generic_map_update_batch()
> error: 'key' dereferencing possible ERR_PTR()
>
> kernel/bpf/syscall.c
> 1296
> 1297 value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
> 1298 if (!value)
> 1299 return -ENOMEM;
> 1300
> 1301 for (cp = 0; cp < max_count; cp++) {
> 1302 key = __bpf_copy_key(keys + cp * map->key_size, map->key_size);
> 1303 if (IS_ERR(key)) {
> 1304 err = PTR_ERR(key);
> 1305 break;
> ^^^^^
> This will Oops.
>
> 1306 }
> 1307 err = -EFAULT;
> 1308 if (copy_from_user(value, values + cp * value_size, value_size))
> 1309 break;
> 1310
> 1311 err = bpf_map_update_value(map, f, key, value,
> 1312 attr->batch.elem_flags);
> 1313
> 1314 if (err)
> 1315 break;
>
> But the success path seems to leak. Anyway, either we free the last
> successful key or we are leaking so this doesn't seem workable. Does
> map->key_size change? Maybe move the allocation from __bpf_copy_key()
> to before the start of the loop.
Thanks for the report, you're right that part is buggy. The allocation
should happen before the loop, and we can do copy_from_user for the
key. I also see the leaking in generic_map_delete_batch, will send the
fixes shortly.
>
> 1316 }
> 1317
> 1318 if (copy_to_user(&uattr->batch.count, &cp, sizeof(cp)))
> 1319 err = -EFAULT;
> 1320
> 1321 kfree(value);
> 1322 kfree(key);
> 1323 return err;
> 1324 }
>
> regards,
> dan carpenter
