On Mon, 2023-12-18 at 21:54 -0500, Andrei Matei wrote:
> On Mon, Dec 18, 2023 at 7:04 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Sat, 2023-12-16 at 20:06 -0500, Andrei Matei wrote:
> > [...]
> >
> > > (*) Besides standing to reason that the checks for a bigger size access
> > > are a super-set of the checks for a smaller size access, I have also
> > > mechanically verified this by reading the code for all types of
> > > pointers. I could convince myself that it's true for all but
> > > PTR_TO_BTF_ID (check_ptr_to_btf_access). There, simply looking
> > > line-by-line does not immediately prove what we want. If anyone has any
> > > qualms, let me know.
> >
> > check_help_mem_access() is a bit obfuscated :)
> > After staring at it for a bit I have a question regarding
> > check_ptr_to_btf_access():
> > - it can call btf_struct_access(),
> > which in can call btf_struct_walk(),
> > which has the following check:
> >
> > if (btf_type_is_ptr(mtype)) {
> > const struct btf_type *stype, *t;
> > enum bpf_type_flag tmp_flag = 0;
> > u32 id;
> >
> > if (msize != size || off != moff) {
> > bpf_log(log,
> > "cannot access ptr member %s with moff %u in struct %s with off %u size %u\n",
> > mname, moff, tname, off, size);
> > return -EACCES;
> > }
> >
> > - previously this code was executed twice, for size 0 and for size
> > umax_value of the size register;
> > - now this code is executed only for umax_value of the size register;
> > - is it possible that with size 0 this code could have reported error
> > -EACCESS error, which would be missed now?
>
> I don't have a good answer. I too have looked at check_ptr_to_btf_access() and
> ended up confused -- but then again, I don't know what's supposed to be allowed
> and what's supposed to not be allowed. I will say, though, that I don't think
> the code as it stands make sense, and I don't think any interaction between the
> zero-size check and btf access is intentional. Around [1] we've looked a bit at
> the history of this zero-size check, and it's been there forever, predating
> most of the code around it. What convinces me personally that the zero-size
> check was not load-bearing is the fact that we were only performing
> the check iff
> umin == 0 -- we were not consistently performing a check for the umin value.
> Also, obviously, we were not performing a check for every possible value in
> between umin and umax. So I can't really imagine positive benefits of the
> inconsistent check we were doing. But then again, I cannot actually speak with
> confidence about it.
Not checking consistently for all possible offsets is a good argument, thank you.
> As a btw, I'll say that we don't allow variable-offset accesses to btf ptr [2].
> I don't know if this should influence how we treat the access size... but
> maybe? Like, should we disallow variable-sized accesses on the same argument as
> disallowing variable-offset ones (whatever that argument may be)? I don't know
> what I'm talking about (generally BTF is foreign to me), but I imagine this all
> means that currently the verifier allows one to read from an array field by
> starting at a compile-time constant offset, and extending to a variable size.
> However, you cannot start from an arbitrary offset, though. Does this
> combination of being strict about the offset but permissive about the size make
> sense?
I agree with you, that disallowing variable size access in BTF case
might make sense. check_ptr_to_btf_access() calls either:
a. env->ops->btf_struct_access(), which is one of the following:
1. _tc_cls_act_btf_struct_access() (through a function pointer),
which allows accessing exactly one field: struct nf_conn->mark;
2. bpf_tcp_ca_btf_struct_access, which allows accessing several
fields in sock, tcp_sock and inet_connection_sock structures.
b. btf_struct_access(), which checks the following:
1. part with btf_find_struct_meta() checks that access does not reach
to some forbidden field;
2. btf_struct_walk() checks that offset and size of the access match
offset and size of some field in the target BTF structure;
Technically, checks a.1, a.2 and b.1 are ok with variable size access,
but b.2 is not and it does not seem to be verified.
I tried a patch below and test_progs seem to pass locally
(but I have some troubles with my local setup at the moment,
so it should be double-checked).
> I'll take guidance. If people prefer we don't touch this code at all, that's
> fine. Although it doesn't feel good to be driven simply by fear.
Would be good if others could comment.
[...]
---
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index cf2a09408bdc..946415d11338 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7328,6 +7328,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
{
int err;
const bool size_is_const = tnum_is_const(reg->var_off);
+ struct bpf_reg_state *ptr_reg = &cur_regs(env)[regno - 1];
/* This is used to refine r0 return value bounds for helpers
* that enforce this value as an upper bound on return values.
@@ -7373,6 +7374,13 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
verbose(env, "verifier bug: !zero_size_allowed should have been handled already\n");
return -EFAULT;
}
+
+ if (base_type(ptr_reg->type) == PTR_TO_BTF_ID && !size_is_const) {
+ verbose(env, "variable length access to r%d %s is not allowed",
+ regno - 1, reg_type_str(env, ptr_reg->type));
+ return -EACCES;
+ }
+
err = check_helper_mem_access(env, regno - 1,
reg->umax_value,
/* zero_size_allowed: we asserted above that umax_value is
