[bpf-next,v4,4/8] bpf: Improve check_raw_mode_ok test for MEM_UNINIT-tagged types

Commit Message

Daniel Borkmann Sept. 6, 2024, 1:56 p.m. UTC

When checking malformed helper function signatures, also take other argument
types into account aside from just ARG_PTR_TO_UNINIT_MEM.

This concerns (formerly) ARG_PTR_TO_{INT,LONG} given uninitialized memory can
be passed there, too.

The func proto sanity check goes back to commit 435faee1aae9 ("bpf, verifier:
add ARG_PTR_TO_RAW_STACK type"), and its purpose was to detect wrong func protos
which had more than just one MEM_UNINIT-tagged type as arguments.

The reason more than one is currently not supported is as we mark stack slots with
STACK_MISC in check_helper_call() in case of raw mode based on meta.access_size to
allow uninitialized stack memory to be passed to helpers when they just write into
the buffer.

Probing for base type as well as MEM_UNINIT tagging ensures that other types do not
get missed (as it used to be the case for ARG_PTR_TO_{INT,LONG}).

Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
Reported-by: Shung-Hsi Yu <[e-mail]>
Signed-off-by: Daniel Borkmann <[e-mail]>
---
 v1 -> v2:
 - new patch (Shung-Hsi)
 v2 -> v3:
 - base_type(type) was needed also

 kernel/bpf/verifier.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Andrii Nakryiko Sept. 6, 2024, 9:03 p.m. UTC | #1

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <[e-mail]> wrote:
>
> When checking malformed helper function signatures, also take other argument
> types into account aside from just ARG_PTR_TO_UNINIT_MEM.
>
> This concerns (formerly) ARG_PTR_TO_{INT,LONG} given uninitialized memory can
> be passed there, too.
>
> The func proto sanity check goes back to commit 435faee1aae9 ("bpf, verifier:
> add ARG_PTR_TO_RAW_STACK type"), and its purpose was to detect wrong func protos
> which had more than just one MEM_UNINIT-tagged type as arguments.
>
> The reason more than one is currently not supported is as we mark stack slots with
> STACK_MISC in check_helper_call() in case of raw mode based on meta.access_size to
> allow uninitialized stack memory to be passed to helpers when they just write into
> the buffer.
>
> Probing for base type as well as MEM_UNINIT tagging ensures that other types do not
> get missed (as it used to be the case for ARG_PTR_TO_{INT,LONG}).
>
> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> Reported-by: Shung-Hsi Yu <[e-mail]>
> Signed-off-by: Daniel Borkmann <[e-mail]>
> ---
>  v1 -> v2:
>  - new patch (Shung-Hsi)
>  v2 -> v3:
>  - base_type(type) was needed also
>
>  kernel/bpf/verifier.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>

LGTM (though I like straight unwrapped lines ;))

Acked-by: Andrii Nakryiko <[e-mail]>


> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3a0801933a79..a1bbe4b401af 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8291,6 +8291,12 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type)
>                type == ARG_CONST_SIZE_OR_ZERO;
>  }
>
> +static bool arg_type_is_raw_mem(enum bpf_arg_type type)
> +{
> +       return base_type(type) == ARG_PTR_TO_MEM &&
> +              type & MEM_UNINIT;
> +}
> +
>  static bool arg_type_is_release(enum bpf_arg_type type)
>  {
>         return type & OBJ_RELEASE;
> @@ -9343,15 +9349,15 @@ static bool check_raw_mode_ok(const struct bpf_func_proto *fn)
>  {
>         int count = 0;
>
> -       if (fn->arg1_type == ARG_PTR_TO_UNINIT_MEM)
> +       if (arg_type_is_raw_mem(fn->arg1_type))
>                 count++;
> -       if (fn->arg2_type == ARG_PTR_TO_UNINIT_MEM)
> +       if (arg_type_is_raw_mem(fn->arg2_type))
>                 count++;
> -       if (fn->arg3_type == ARG_PTR_TO_UNINIT_MEM)
> +       if (arg_type_is_raw_mem(fn->arg3_type))
>                 count++;
> -       if (fn->arg4_type == ARG_PTR_TO_UNINIT_MEM)
> +       if (arg_type_is_raw_mem(fn->arg4_type))
>                 count++;
> -       if (fn->arg5_type == ARG_PTR_TO_UNINIT_MEM)
> +       if (arg_type_is_raw_mem(fn->arg5_type))
>                 count++;
>
>         /* We only support one arg being in raw mode at the moment,
> --
> 2.43.0
>

Shung-Hsi Yu Sept. 9, 2024, 12:24 p.m. UTC | #2

On Fri, Sep 06, 2024 at 03:56:04PM GMT, Daniel Borkmann wrote:
> When checking malformed helper function signatures, also take other argument
> types into account aside from just ARG_PTR_TO_UNINIT_MEM.
> 
> This concerns (formerly) ARG_PTR_TO_{INT,LONG} given uninitialized memory can
> be passed there, too.
> 
> The func proto sanity check goes back to commit 435faee1aae9 ("bpf, verifier:
> add ARG_PTR_TO_RAW_STACK type"), and its purpose was to detect wrong func protos
> which had more than just one MEM_UNINIT-tagged type as arguments.
> 
> The reason more than one is currently not supported is as we mark stack slots with
> STACK_MISC in check_helper_call() in case of raw mode based on meta.access_size to
> allow uninitialized stack memory to be passed to helpers when they just write into
> the buffer.
> 
> Probing for base type as well as MEM_UNINIT tagging ensures that other types do not
> get missed (as it used to be the case for ARG_PTR_TO_{INT,LONG}).
> 
> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> Reported-by: Shung-Hsi Yu <[e-mail]>
> Signed-off-by: Daniel Borkmann <[e-mail]>

Acked-by: Shung-Hsi Yu <[e-mail]>

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3a0801933a79..a1bbe4b401af 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8291,6 +8291,12 @@  static bool arg_type_is_mem_size(enum bpf_arg_type type)
 	       type == ARG_CONST_SIZE_OR_ZERO;
 }
 
+static bool arg_type_is_raw_mem(enum bpf_arg_type type)
+{
+	return base_type(type) == ARG_PTR_TO_MEM &&
+	       type & MEM_UNINIT;
+}
+
 static bool arg_type_is_release(enum bpf_arg_type type)
 {
 	return type & OBJ_RELEASE;
@@ -9343,15 +9349,15 @@  static bool check_raw_mode_ok(const struct bpf_func_proto *fn)
 {
 	int count = 0;
 
-	if (fn->arg1_type == ARG_PTR_TO_UNINIT_MEM)
+	if (arg_type_is_raw_mem(fn->arg1_type))
 		count++;
-	if (fn->arg2_type == ARG_PTR_TO_UNINIT_MEM)
+	if (arg_type_is_raw_mem(fn->arg2_type))
 		count++;
-	if (fn->arg3_type == ARG_PTR_TO_UNINIT_MEM)
+	if (arg_type_is_raw_mem(fn->arg3_type))
 		count++;
-	if (fn->arg4_type == ARG_PTR_TO_UNINIT_MEM)
+	if (arg_type_is_raw_mem(fn->arg4_type))
 		count++;
-	if (fn->arg5_type == ARG_PTR_TO_UNINIT_MEM)
+	if (arg_type_is_raw_mem(fn->arg5_type))
 		count++;
 
 	/* We only support one arg being in raw mode at the moment,