[bug report] bpf: Add generic support for update and delete batch ops

[bug report] bpf: Add generic support for update and delete batch ops

[Dan Carpenter]

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.

  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

Re: [bug report] bpf: Add generic support for update and delete batch ops

[Brian Vazquez]

On Fri, Jan 17, 2020 at 2:44 AM Dan Carpenter <dan.carpenter@oracle.com> 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