On Thu, 28 Nov 2024 at 05:13, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2024-11-27 at 08:58 -0800, Kumar Kartikeya Dwivedi wrote:
>
> Overall looks good, but please take a look at a few notes below.
>
> [...]
>
> > @@ -1349,77 +1350,69 @@ static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state
> > * On success, returns a valid pointer id to associate with the register
> > * On failure, returns a negative errno.
> > */
> > -static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
> > +static struct bpf_reference_state *acquire_reference_state(struct bpf_verifier_env *env, int insn_idx, bool gen_id)
> > {
> > struct bpf_verifier_state *state = env->cur_state;
> > int new_ofs = state->acquired_refs;
> > - int id, err;
> > + int err;
> >
> > err = resize_reference_state(state, state->acquired_refs + 1);
> > if (err)
> > - return err;
> > - id = ++env->id_gen;
> > - state->refs[new_ofs].type = REF_TYPE_PTR;
> > - state->refs[new_ofs].id = id;
> > + return NULL;
> > + if (gen_id)
> > + state->refs[new_ofs].id = ++env->id_gen;
>
> Nit: state->refs[new_ods].id might end up with garbage value if 'gen_id' is false.
> The resize_reference_state() uses realloc_array(),
> which allocates memory with GFP_KERNEL, but without __GFP_ZERO flag.
> This is not a problem with current patch, as you always check
> reference type before checking id, but most of the data strucures
> in verifier are zero initialized just in case.
We end up assigning to s->id if gen_id is false, e.g.
acquire_lock_state, so I think we'll be fine without __GFP_ZERO.
>
> > state->refs[new_ofs].insn_idx = insn_idx;
> >
> > - return id;
> > + return &state->refs[new_ofs];
> > +}
>
> [...]
>
> > -/* release function corresponding to acquire_reference_state(). Idempotent. */
> > -static int release_reference_state(struct bpf_verifier_state *state, int ptr_id)
> > +static void release_reference_state(struct bpf_verifier_state *state, int idx)
> > {
> > - int i, last_idx;
> > + int last_idx;
> >
> > last_idx = state->acquired_refs - 1;
> > - for (i = 0; i < state->acquired_refs; i++) {
> > - if (state->refs[i].type != REF_TYPE_PTR)
> > - continue;
> > - if (state->refs[i].id == ptr_id) {
> > - if (last_idx && i != last_idx)
> > - memcpy(&state->refs[i], &state->refs[last_idx],
> > - sizeof(*state->refs));
> > - memset(&state->refs[last_idx], 0, sizeof(*state->refs));
> > - state->acquired_refs--;
> > - return 0;
> > - }
> > - }
> > - return -EINVAL;
> > + if (last_idx && idx != last_idx)
> > + memcpy(&state->refs[idx], &state->refs[last_idx], sizeof(*state->refs));
> > + memset(&state->refs[last_idx], 0, sizeof(*state->refs));
> > + state->acquired_refs--;
> > + return;
> > }
>
> Such implementation replaces element at 'idx' with element at 'last_idx'.
> If the intention is to use 'state->refs' as a stack of acquired irq flags,
> the stack property would be broken by this trick.
> E.g. consider array [a, b, c, d] where 'idx' points to 'b',
> after release_reference_state() the array would become [a, d, c].
> You need to do 'memmove' instead.
>
Wow, great catch. Thanks for spotting this. I'll fix this and let me
see if I can add a selftest that would've triggered this particular
pattern.
> [...]
>
> > @@ -9666,21 +9659,41 @@ static void mark_pkt_end(struct bpf_verifier_state *vstate, int regn, bool range
> > reg->range = AT_PKT_END;
> > }
> >
> > +static int release_reference_nomark(struct bpf_verifier_state *state, int ref_obj_id)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < state->acquired_refs; i++) {
> > + if (state->refs[i].type != REF_TYPE_PTR)
> > + continue;
> > + if (state->refs[i].id == ref_obj_id) {
> > + release_reference_state(state, i);
> > + return 0;
> > + }
> > + }
> > + return -EINVAL;
> > +}
> > +
> > /* The pointer with the specified id has released its reference to kernel
> > * resources. Identify all copies of the same pointer and clear the reference.
> > + *
> > + * This is the release function corresponding to acquire_reference(). Idempotent.
> > + * The 'mark' boolean is used to optionally skip scrubbing registers matching
> ^^^^^^
> Nit: this is probably a remnant of some older patch revision,
> function no longer takes 'mark' parameter.
Yeah, this is a leftover. Sorry about that. Will fix.
>
> > + * the ref_obj_id, in case they need to be switched to some other type instead
> > + * of havoc scalar value.
> > */
> > -static int release_reference(struct bpf_verifier_env *env,
> > - int ref_obj_id)
> > +static int release_reference(struct bpf_verifier_env *env, int ref_obj_id)
> > {
>
> [...]
>
