Message-ID: <Y1DmnQE0xuj1RDp7@maniforge.dhcp.thefacebook.com>
Date:   Thu, 20 Oct 2022 01:11:41 -0500
From:   David Vernet <void@...ifault.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
        yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org,
        sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org,
        linux-kernel@...r.kernel.org, kernel-team@...com, tj@...nel.org
Subject: Re: [PATCH v5 1/3] bpf: Allow trusted pointers to be passed to
 KF_TRUSTED_ARGS kfuncs

On Thu, Oct 20, 2022 at 11:27:49AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Sure, I can make that change. We'll have to increase the size of the
> > prefix string on the stack, but that's hardly problematic as these
> > strings are not terribly large.
> >
> > > > @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> > > >         if (type_flag(reg->type) & PTR_UNTRUSTED)
> > > >                 flag |= PTR_UNTRUSTED;
> > > >
> > > > +       /* All pointers obtained by walking a struct are nested. */
> > > > +       flag |= PTR_NESTED;
> > > > +
> > >
> > > Instead of PTR_NESTED, how about PTR_WALK?
> >
> > I don't have a strong preference between either, though I would prefer
> > PTR_WALKED if we go with the latter. Does that work for you?
> >
> 
> Yes, I just think PTR_NESTED is a bit misleading, it's not nested within the old
> object, we loaded a pointer from it, it should just indicate that the pointer
> came from a walk of a trusted PTR_TO_BTF_ID.

Ok, we'll go with PTR_WALKED.

> > > > [...]
> > > > @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
> > > >  static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
> > > >  static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
> > > >  static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> > > > -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> > > > +static const struct bpf_reg_types btf_ptr_types = {
> > > > +       .types = {
> > > > +               PTR_TO_BTF_ID,
> > > > +               PTR_TO_BTF_ID | PTR_NESTED
> > > > +       },
> > > > +};
> > >
> > > CI fails, two of those failures are from not updating
> > > check_func_arg_reg_off for PTR_TO_BTF_ID | PTR_WALK, and the other one
> >
> > Gah, I didn't think it was necessary for this case as it's not required
> > for btf_check_func_arg_match(), which will eventually just fail in the
> > following check:
> >
> > 	if (!btf_type_is_struct(ref_t)) {
> > 		bpf_log(log, "kernel function %s args#%d pointer type %s %s is not support
> > 			func_name, i, btf_type_str(ref_t),
> > 			ref_tname);
> > 		return -EINVAL;
> > 	}
> 
> Why would it fail there? It will still be a struct type.
> I think you misunderstand this a bit.

Apologies, as mentioned below I pasted the wrong if-check on accident.
If I had actually meant to paste that one, then saying I "misunderstand
this a bit" would have been a very generous understatment :-)

> When you have task from tracing ctx arg:
> r1 = ctx;
> r1 = *(r1 + ...); // PTR_TO_BTF_ID, task_struct, off=0
> // r1 = task->next
> r1 = *(r1 + offsetof(task_struct, next)); // PTR_TO_BTF_ID | PTR_WALKED, task_struct, off = 0
> 
> We loaded a pointer from task_struct into r1.
> Now r1 still points to a task_struct, so that check above won't fail for r1.

I meant to paste the if-condition _above_ that one. This is the if-check
we'll fail due to the presence of a type modifier (PTR_WALKED):

	} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
		   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
		const struct btf_type *reg_ref_t;
		const struct btf *reg_btf;
		const char *reg_ref_tname;
		u32 reg_ref_id;

So we'll never even get to the if check I originally pasted because
reg->type == PTR_TO_BTF_ID will fail for a PTR_WALKED reg. And then
below we'll eventually fail later on here:

	/* Permit pointer to mem, but only when argument
	 * type is pointer to scalar, or struct composed
	 * (recursively) of scalars.
	 * When arg_mem_size is true, the pointer can be
	 * void *.
	 * Also permit initialized local dynamic pointers.
	 */
	if (!btf_type_is_scalar(ref_t) &&
	    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
	    !arg_dynptr &&
	    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
		bpf_log(log,
			"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
			i, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
		return -EINVAL;
	}

Appreciate the explanation, sorry to have made you type it.

> > Note that we also don't include PTR_TO_BTF_ID | PTR_UNTRUSTED here. The
> > difference for PTR_TO_BTF_ID | PTR_WALK(ED) is of course that we also need to
> > allow it to work properly for normal helper calls, so I'll make that change.
> > Thanks for pointing it out. In general, the whole dance between register base
> > types + modifiers sometimes feels like a mine field...
> >
> 
> Yes, I don't like how it's growing and being mixed either. Eventually I think we
> should document what combinations are allowed and reject everything else when
> initializing reg->type to prevent bugs, but IDK whether something like this
> would be accepted.

That seems like a pretty sane idea. A project for another day...

Powered by blists - more mailing lists