Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 Jan 2023 21:44:41 -0800
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     David Vernet <void@manifault.com>
Cc:     Kumar Kartikeya Dwivedi <memxor@gmail.com>, bpf@vger.kernel.org,
        ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        martin.lau@linux.dev, song@kernel.org, yhs@meta.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@meta.com, tj@kernel.org
Subject: Re: [PATCH bpf-next 3/8] bpf: Disallow NULL PTR_TO_MEM for trusted
 kfuncs
Message-ID: <20230120054441.arj5h6yrnh5jsrgr@MacBook-Pro-6.local.dhcp.thefacebook.com>
References: <20230119235833.2948341-1-void@manifault.com>
 <20230119235833.2948341-4-void@manifault.com>
 <20230120052101.sevhc4jybcm6onu2@apollo>
 <Y8onR2T2zmMU6MmH@maniforge.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8onR2T2zmMU6MmH@maniforge.lan>
Precedence: bulk

On Thu, Jan 19, 2023 at 11:31:51PM -0600, David Vernet wrote:
> On Fri, Jan 20, 2023 at 10:51:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Fri, Jan 20, 2023 at 05:28:28AM IST, David Vernet wrote:
> > > KF_TRUSTED_ARGS kfuncs currently have a subtle and insidious bug in
> > > validating pointers to scalars. Say that you have a kfunc like the
> > > following, which takes an array as the first argument:
> > >
> > > bool bpf_cpumask_empty(const struct cpumask *cpumask)
> > > {
> > > 	return cpumask_empty(cpumask);
> > > }
> > >
> > > ...
> > > BTF_ID_FLAGS(func, bpf_cpumask_empty, KF_TRUSTED_ARGS)
> > > ...
> > >
> > 
> > This is known and expected.
> 
> Expected? So kfuncs are expected to always check whether any pointer to
> a scalar is non-NULL? Seems like a poor UX. I like your suggestion below
> to address it so it's opt-in.

I'm confused as well.
KF_TRUSTED_ARGS means that all arguments are valid and != NULL.
From our doc:
"
The KF_TRUSTED_ARGS flag is used for kfuncs taking pointer arguments. It
indicates that the all pointer arguments are valid, and that all pointers to
BTF objects have been passed in their unmodified form (that is, at a zero
"

That includes that arguments are guaranted to be != NULL.

> > > If a BPF program were to invoke the kfunc with a NULL argument, it would
> > > crash the kernel. The reason is that struct cpumask is defined as a
> > > bitmap, which is itself defined as an array, and is accessed as a memory
> > > address memory by bitmap operations. So when the verifier analyzes the
> > > register, it interprets it as a pointer to a scalar struct, which is an
> > > array of size 8. check_mem_reg() then sees that the register is NULL,
> > > and returns 0, and the kfunc crashes when it passes it down to the
> > > cpumask wrappers.
> > >
> > > To fix this, this patch adds a check for KF_ARG_PTR_TO_MEM which
> > > verifies that the register doesn't contain a NULL pointer if the kfunc
> > > is KF_TRUSTED_ARGS.
> > >
> > > This may or may not be desired behavior. Some kfuncs may want to
> > > allow callers to pass NULL-able pointers. An alternative would be adding
> > > a KF_NOT_NULL flag and leaving KF_TRUSTED_ARGS alone, though given that
> > > a kfunc is saying it wants to "trust" an argument, it seems reasonable
> > > to prevent NULL.
> > >
> > > Signed-off-by: David Vernet <void@manifault.com>
> > > ---
> > >  kernel/bpf/verifier.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 9fa101420046..28ccb92ebe65 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -9092,6 +9092,11 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > >  					i, btf_type_str(ref_t), ref_tname, PTR_ERR(resolve_ret));
> > >  				return -EINVAL;
> > >  			}
> > > +			if (is_kfunc_trusted_args(meta) && register_is_null(reg)) {
> > > +				verbose(env, "NULL pointer passed to trusted arg%d\n", i);
> > > +				return -EACCES;
> > > +			}
> > > +
> > 
> > Current patch looks like a stop gap solution. Just checking for register_is_null
> > is not enough, what about PTR_MAYBE_NULL? That can also be passed. Some
> > arguments can be both PTR_TO_BTF_ID and PTR_TO_MEM, so it will be bypassed in
> > the other case because this check is limited to KF_ARG_PTR_TO_MEM. It would
> 
> This wouldn't happen if you had a PTR_TO_BTF_ID, would it? In that case
> you could just rely on PTR_TRUSTED. IMO that really should be the
> default for any pointer argument. If you have KF_ARGS_TRUSTED, the kfunc
> should just be able to assume that the pointers have been verified.

+1

> Regardless, you're right that this isn't a complete solution because of
> PTR_MAYBE_NULL. I'm fine with adding an __or_null suffix that allows
> NULL, and we disallow NULL or PTR_MAYBE_NULL from any KF_TRUSTED_ARGS
> argument otherwise. Or we just also disallow PTR_MAYBE_NULL and try to
> hold off on adding yet another suffix until we have proper per-arg kfunc
> definitions.

PTR_MAYBE_NULL should not be allowed into kfunc with KF_TRUSTED_ARGS.