Re: [PATCH bpf-next 2/3] bpf: Add verifier checks for bpf_timer.
From: Andrii Nakryiko <hidden>
Date: 2021-06-02 22:35:54
Also in:
bpf
On Wed, May 26, 2021 at 9:03 PM Alexei Starovoitov [off-list ref] wrote:
From: Alexei Starovoitov <ast@kernel.org> Add appropriate safety checks for bpf_timer: - restrict to array, hash, lru. per-cpu maps cannot be supported. - kfree bpf_timer during map_delete_elem and map_free. - verifier btf checks. - safe interaction with lookup/update/delete operations and iterator. - relax the first field only requirement of the previous patch. - allow bpf_timer in global data and search for it in datasec. - check prog_rdonly, frozen flags. - mmap is allowed. otherwise global timer is not possible. Signed-off-by: Alexei Starovoitov <ast@kernel.org> --- include/linux/bpf.h | 36 +++++++++++++----- include/linux/btf.h | 1 + kernel/bpf/arraymap.c | 7 ++++ kernel/bpf/btf.c | 77 +++++++++++++++++++++++++++++++------- kernel/bpf/hashtab.c | 53 ++++++++++++++++++++------ kernel/bpf/helpers.c | 2 +- kernel/bpf/local_storage.c | 4 +- kernel/bpf/syscall.c | 23 ++++++++++-- kernel/bpf/verifier.c | 30 +++++++++++++-- 9 files changed, 190 insertions(+), 43 deletions(-)
[...]
/* copy everything but bpf_spin_lock */
static inline void copy_map_value(struct bpf_map *map, void *dst, void *src)
{
+ u32 off = 0, size = 0;
+
if (unlikely(map_value_has_spin_lock(map))) {
- u32 off = map->spin_lock_off;
+ off = map->spin_lock_off;
+ size = sizeof(struct bpf_spin_lock);
+ } else if (unlikely(map_value_has_timer(map))) {
+ off = map->timer_off;
+ size = sizeof(struct bpf_timer);
+ }
so the need to handle 0, 1, or 2 gaps seems to be the only reason to
disallow both bpf_spinlock and bpf_timer in one map element, right?
Isn't it worth addressing it from the very beginning to lift the
artificial restriction? E.g., for speed, you'd do:
if (likely(neither spinlock nor timer)) {
/* fastest pass */
} else if (only one of spinlock or timer) {
/* do what you do here */
} else {
int off1, off2, sz1, sz2;
if (spinlock_off < timer_off) {
off1 = spinlock_off;
sz1 = spinlock_sz;
off2 = timer_off;
sz2 = timer_sz;
} else {
... you get the idea
}
memcpy(0, off1);
memcpy(off1+sz1, off2);
memcpy(off2+sz2, total_sz);
}
It's not that bad, right?
+ if (unlikely(size)) {
memcpy(dst, src, off);
- memcpy(dst + off + sizeof(struct bpf_spin_lock),
- src + off + sizeof(struct bpf_spin_lock),
- map->value_size - off - sizeof(struct bpf_spin_lock));
+ memcpy(dst + off + size,
+ src + off + size,
+ map->value_size - off - size);
} else {
memcpy(dst, src, map->value_size);
}[...]
quoted hunk ↗ jump to hunk
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index f386f85aee5c..0a828dc4968e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c@@ -3241,6 +3241,15 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno, return -EACCES; } } + if (map_value_has_timer(map)) { + u32 t = map->timer_off; + + if (reg->smin_value + off < t + sizeof(struct bpf_timer) &&
<= ? Otherwise we allow accessing the first byte, unless I'm mistaken.
quoted hunk ↗ jump to hunk
+ t < reg->umax_value + off + size) { + verbose(env, "bpf_timer cannot be accessed directly by load/store\n"); + return -EACCES; + } + } return err; }@@ -4675,9 +4684,24 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno, map->name); return -EINVAL; } - if (val) { - /* todo: relax this requirement */ - verbose(env, "bpf_timer field can only be first in the map value element\n");
ok, this was confusing, but now I see why you did that...
+ if (!map_value_has_timer(map)) {
+ if (map->timer_off == -E2BIG)
+ verbose(env,
+ "map '%s' has more than one 'struct bpf_timer'\n",
+ map->name);
+ else if (map->timer_off == -ENOENT)
+ verbose(env,
+ "map '%s' doesn't have 'struct bpf_timer'\n",
+ map->name);
+ else
+ verbose(env,
+ "map '%s' is not a struct type or bpf_timer is mangled\n",
+ map->name);
+ return -EINVAL;
+ }
+ if (map->timer_off != val + reg->off) {
+ verbose(env, "off %lld doesn't point to 'struct bpf_timer' that is at %d\n",
+ val + reg->off, map->timer_off);
return -EINVAL;
}
WARN_ON(meta->map_ptr);
--
2.30.2