[bpf-next,3/8] bpf: Disallow NULL PTR_TO_MEM for trusted kfuncs

Commit Message

David Vernet Jan. 19, 2023, 11:58 p.m. UTC

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)
...

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 <[e-mail]>
---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Kumar Kartikeya Dwivedi Jan. 20, 2023, 5:21 a.m. UTC | #1

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.

> 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 <[e-mail]>
> ---
>  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
probably be better to disallow NULL by default and explicitly tag the argument
with __or_null to indicate that NULL is accepted. Seems like a much better
default to me.

David Vernet Jan. 20, 2023, 5:31 a.m. UTC | #2

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.

> > 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 <[e-mail]>
> > ---
> >  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.

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.

> probably be better to disallow NULL by default and explicitly tag the argument
> with __or_null to indicate that NULL is accepted. Seems like a much better
> default to me.

Alexei Starovoitov Jan. 20, 2023, 5:44 a.m. UTC | #3

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 <[e-mail]>
> > > ---
> > >  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.

Patch

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;
+			}
+
 			ret = check_mem_reg(env, reg, regno, type_size);
 			if (ret < 0)
 				return ret;