linux-kernel - Re: [PATCH v5 1/3] bpf: Allow trusted pointers to be passed to KF_TRUSTED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221020062229.b7e7r7lrnkszjoiy@apollo>
Date:   Thu, 20 Oct 2022 11:52:29 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     David Vernet <void@...ifault.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:41:41AM IST, David Vernet wrote:
> [...]
> 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.
>

Ah, I see. Your analysis is right, but the error in CI comes from
check_func_arg_reg_off invocation in check_helper_call, this code is for kfuncs.

Since you have this to preserve backwards compat:
+static const struct bpf_reg_types btf_ptr_types = {
+	.types = {
+		PTR_TO_BTF_ID,
+		PTR_TO_BTF_ID | PTR_NESTED
+	},
+};

It allows passing those with PTR_NESTED to stable helpers.