[PATCH bpf-next v4 1/8] bpf: Fix bpf_strtol and bpf_strtoul helpers for 32bit

[PATCH bpf-next v4 1/8] bpf: Fix bpf_strtol and bpf_strtoul helpers for 32bit

[Daniel Borkmann]

The bpf_strtol() and bpf_strtoul() helpers are currently broken on 32bit:

The argument type ARG_PTR_TO_LONG is BPF-side "long", not kernel-side "long"
and therefore always considered fixed 64bit no matter if 64 or 32bit underlying
architecture.

This contract breaks in case of the two mentioned helpers since their BPF_CALL
definition for the helpers was added with {unsigned,}long *res. Meaning, the
transition from BPF-side "long" (BPF program) to kernel-side "long" (BPF helper)
breaks here.

Both helpers call __bpf_strtoll() with "long long" correctly, but later assigning
the result into 32-bit "*(long *)" on 32bit architectures. From a BPF program
point of view, this means upper bits will be seen as uninitialised.

Therefore, fix both BPF_CALL signatures to {s,u}64 types to fix this situation.

Now, changing also uapi/bpf.h helper documentation which generates bpf_helper_defs.h
for BPF programs is tricky: Changing signatures there to __{s,u}64 would trigger
compiler warnings (incompatible pointer types passing 'long *' to parameter of type
'__s64 *' (aka 'long long *')) for existing BPF programs.

Leaving the signatures as-is would be fine as from BPF program point of view it is
still BPF-side "long" and thus equivalent to __{s,u}64 on 64 or 32bit underlying
architectures.

Note that bpf_strtol() and bpf_strtoul() are the only helpers with this issue.

Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://lore.kernel.org/bpf/481fcec8-c12c-9abb-8ecb-76c71c009959@iogearbox.net
---
 v3 -> v4:
 - added patch

 kernel/bpf/helpers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 3956be5d6440..0cf42be52890 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -518,7 +518,7 @@ static int __bpf_strtoll(const char *buf, size_t buf_len, u64 flags,
 }
 
 BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
-	   long *, res)
+	   s64 *, res)
 {
 	long long _res;
 	int err;
@@ -543,7 +543,7 @@ const struct bpf_func_proto bpf_strtol_proto = {
 };
 
 BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
-	   unsigned long *, res)
+	   u64 *, res)
 {
 	unsigned long long _res;
 	bool is_negative;
-- 
2.43.0

[PATCH bpf-next v4 2/8] bpf: Remove truncation test in bpf_strtol and bpf_strtoul helpers

[Daniel Borkmann]

Both bpf_strtol() and bpf_strtoul() helpers passed a temporary "long long"
respectively "unsigned long long" to __bpf_strtoll() / __bpf_strtoull().

Later, the result was checked for truncation via _res != ({unsigned,} long)_res
as the destination buffer for the BPF helpers was of type {unsigned,} long
which is 32bit on 32bit architectures.

Given the latter was a bug in the helper signatures where the destination buffer
got adjusted to {s,u}64, the truncation check can now be removed.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 v3 -> v4:
 - added patch 

 kernel/bpf/helpers.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 0cf42be52890..5404bb964d83 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -526,8 +526,6 @@ BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
 	err = __bpf_strtoll(buf, buf_len, flags, &_res);
 	if (err < 0)
 		return err;
-	if (_res != (long)_res)
-		return -ERANGE;
 	*res = _res;
 	return err;
 }
@@ -554,8 +552,6 @@ BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
 		return err;
 	if (is_negative)
 		return -EINVAL;
-	if (_res != (unsigned long)_res)
-		return -ERANGE;
 	*res = _res;
 	return err;
 }
-- 
2.43.0

Re: [PATCH bpf-next v4 2/8] bpf: Remove truncation test in bpf_strtol and bpf_strtoul helpers

[Andrii Nakryiko]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Both bpf_strtol() and bpf_strtoul() helpers passed a temporary "long long"
> respectively "unsigned long long" to __bpf_strtoll() / __bpf_strtoull().
>
> Later, the result was checked for truncation via _res != ({unsigned,} long)_res
> as the destination buffer for the BPF helpers was of type {unsigned,} long
> which is 32bit on 32bit architectures.
>
> Given the latter was a bug in the helper signatures where the destination buffer
> got adjusted to {s,u}64, the truncation check can now be removed.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  v3 -> v4:
>  - added patch
>

Acked-by: Andrii Nakryiko <andrii@kernel.org>


>  kernel/bpf/helpers.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 0cf42be52890..5404bb964d83 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -526,8 +526,6 @@ BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
>         err = __bpf_strtoll(buf, buf_len, flags, &_res);
>         if (err < 0)
>                 return err;
> -       if (_res != (long)_res)
> -               return -ERANGE;
>         *res = _res;
>         return err;
>  }
> @@ -554,8 +552,6 @@ BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
>                 return err;
>         if (is_negative)
>                 return -EINVAL;
> -       if (_res != (unsigned long)_res)
> -               return -ERANGE;
>         *res = _res;
>         return err;
>  }
> --
> 2.43.0
>

[PATCH bpf-next v4 3/8] bpf: Fix helper writes to read-only maps

[Daniel Borkmann]

Lonial found an issue that despite user- and BPF-side frozen BPF map
(like in case of .rodata), it was still possible to write into it from
a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
as arguments.

In check_func_arg() when the argument is as mentioned, the meta->raw_mode
is never set. Later, check_helper_mem_access(), under the case of
PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
subsequent call to check_map_access_type() and given the BPF map is
read-only it succeeds.

The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
when results are written into them as opposed to read out of them. The
latter indicates that it's okay to pass a pointer to uninitialized memory
as the memory is written to anyway.

However, ARG_PTR_TO_{LONG,INT} is a special case of ARG_PTR_TO_FIXED_SIZE_MEM
just with additional alignment requirement. So it is better to just get
rid of the ARG_PTR_TO_{LONG,INT} special cases altogether and reuse the
fixed size memory types. For this, add MEM_ALIGNED to additionally ensure
alignment given these helpers write directly into the args via *<ptr> = val.
The .arg*_size has been initialized reflecting the actual sizeof(*<ptr>).

MEM_ALIGNED can only be used in combination with MEM_FIXED_SIZE annotated
argument types, since in !MEM_FIXED_SIZE cases the verifier does not know
the buffer size a priori and therefore cannot blindly write *<ptr> = val.

Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
Reported-by: Lonial Con <kongln9170@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 v1 -> v2:
 - switch to MEM_FIXED_SIZE
 v3 -> v4:
 - fixed 64bit in strto{u,}l (Alexei)

 include/linux/bpf.h      |  7 +++++--
 kernel/bpf/helpers.c     |  8 ++++++--
 kernel/bpf/syscall.c     |  4 +++-
 kernel/bpf/verifier.c    | 38 +++++---------------------------------
 kernel/trace/bpf_trace.c |  8 ++++++--
 net/core/filter.c        |  8 ++++++--
 6 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6f87fb014fba..6a61ed4266b6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -695,6 +695,11 @@ enum bpf_type_flag {
 	/* DYNPTR points to xdp_buff */
 	DYNPTR_TYPE_XDP		= BIT(16 + BPF_BASE_TYPE_BITS),
 
+	/* Memory must be aligned on some architectures, used in combination with
+	 * MEM_FIXED_SIZE.
+	 */
+	MEM_ALIGNED		= BIT(17 + BPF_BASE_TYPE_BITS),
+
 	__BPF_TYPE_FLAG_MAX,
 	__BPF_TYPE_LAST_FLAG	= __BPF_TYPE_FLAG_MAX - 1,
 };
@@ -732,8 +737,6 @@ enum bpf_arg_type {
 	ARG_ANYTHING,		/* any (initialized) argument is ok */
 	ARG_PTR_TO_SPIN_LOCK,	/* pointer to bpf_spin_lock */
 	ARG_PTR_TO_SOCK_COMMON,	/* pointer to sock_common */
-	ARG_PTR_TO_INT,		/* pointer to int */
-	ARG_PTR_TO_LONG,	/* pointer to long */
 	ARG_PTR_TO_SOCKET,	/* pointer to bpf_sock (fullsock) */
 	ARG_PTR_TO_BTF_ID,	/* pointer to in-kernel struct */
 	ARG_PTR_TO_RINGBUF_MEM,	/* pointer to dynamically reserved ringbuf memory */
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 5404bb964d83..0587d0c2375a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -537,7 +537,9 @@ const struct bpf_func_proto bpf_strtol_proto = {
 	.arg1_type	= ARG_PTR_TO_MEM | MEM_RDONLY,
 	.arg2_type	= ARG_CONST_SIZE,
 	.arg3_type	= ARG_ANYTHING,
-	.arg4_type	= ARG_PTR_TO_LONG,
+	.arg4_type	= ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg4_size	= sizeof(s64),
 };
 
 BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
@@ -563,7 +565,9 @@ const struct bpf_func_proto bpf_strtoul_proto = {
 	.arg1_type	= ARG_PTR_TO_MEM | MEM_RDONLY,
 	.arg2_type	= ARG_CONST_SIZE,
 	.arg3_type	= ARG_ANYTHING,
-	.arg4_type	= ARG_PTR_TO_LONG,
+	.arg4_type	= ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg4_size	= sizeof(u64),
 };
 
 BPF_CALL_3(bpf_strncmp, const char *, s1, u32, s1_sz, const char *, s2)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fc62f5c4faf9..feb276771c03 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5954,7 +5954,9 @@ static const struct bpf_func_proto bpf_kallsyms_lookup_name_proto = {
 	.arg1_type	= ARG_PTR_TO_MEM,
 	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
 	.arg3_type	= ARG_ANYTHING,
-	.arg4_type	= ARG_PTR_TO_LONG,
+	.arg4_type	= ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg4_size	= sizeof(u64),
 };
 
 static const struct bpf_func_proto *
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 53d0556fbbf3..3a0801933a79 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8301,16 +8301,6 @@ static bool arg_type_is_dynptr(enum bpf_arg_type type)
 	return base_type(type) == ARG_PTR_TO_DYNPTR;
 }
 
-static int int_ptr_type_to_size(enum bpf_arg_type type)
-{
-	if (type == ARG_PTR_TO_INT)
-		return sizeof(u32);
-	else if (type == ARG_PTR_TO_LONG)
-		return sizeof(u64);
-
-	return -EINVAL;
-}
-
 static int resolve_map_arg_type(struct bpf_verifier_env *env,
 				 const struct bpf_call_arg_meta *meta,
 				 enum bpf_arg_type *arg_type)
@@ -8383,16 +8373,6 @@ static const struct bpf_reg_types mem_types = {
 	},
 };
 
-static const struct bpf_reg_types int_ptr_types = {
-	.types = {
-		PTR_TO_STACK,
-		PTR_TO_PACKET,
-		PTR_TO_PACKET_META,
-		PTR_TO_MAP_KEY,
-		PTR_TO_MAP_VALUE,
-	},
-};
-
 static const struct bpf_reg_types spin_lock_types = {
 	.types = {
 		PTR_TO_MAP_VALUE,
@@ -8453,8 +8433,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_SPIN_LOCK]		= &spin_lock_types,
 	[ARG_PTR_TO_MEM]		= &mem_types,
 	[ARG_PTR_TO_RINGBUF_MEM]	= &ringbuf_mem_types,
-	[ARG_PTR_TO_INT]		= &int_ptr_types,
-	[ARG_PTR_TO_LONG]		= &int_ptr_types,
 	[ARG_PTR_TO_PERCPU_BTF_ID]	= &percpu_btf_ptr_types,
 	[ARG_PTR_TO_FUNC]		= &func_ptr_types,
 	[ARG_PTR_TO_STACK]		= &stack_ptr_types,
@@ -9020,6 +8998,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			err = check_helper_mem_access(env, regno,
 						      fn->arg_size[arg], false,
 						      meta);
+			if (err)
+				return err;
+			if (arg_type & MEM_ALIGNED)
+				err = check_ptr_alignment(env, reg, 0,
+							  fn->arg_size[arg], true);
 		}
 		break;
 	case ARG_CONST_SIZE:
@@ -9044,17 +9027,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		if (err)
 			return err;
 		break;
-	case ARG_PTR_TO_INT:
-	case ARG_PTR_TO_LONG:
-	{
-		int size = int_ptr_type_to_size(arg_type);
-
-		err = check_helper_mem_access(env, regno, size, false, meta);
-		if (err)
-			return err;
-		err = check_ptr_alignment(env, reg, 0, size, true);
-		break;
-	}
 	case ARG_PTR_TO_CONST_STR:
 	{
 		err = check_reg_const_str(env, reg, regno);
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 98e395f1baae..b444f3d8e2f8 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1202,7 +1202,9 @@ static const struct bpf_func_proto bpf_get_func_arg_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_PTR_TO_CTX,
 	.arg2_type	= ARG_ANYTHING,
-	.arg3_type	= ARG_PTR_TO_LONG,
+	.arg3_type	= ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg3_size	= sizeof(u64),
 };
 
 BPF_CALL_2(get_func_ret, void *, ctx, u64 *, value)
@@ -1218,7 +1220,9 @@ static const struct bpf_func_proto bpf_get_func_ret_proto = {
 	.func		= get_func_ret,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_PTR_TO_CTX,
-	.arg2_type	= ARG_PTR_TO_LONG,
+	.arg2_type	= ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg2_size	= sizeof(u64),
 };
 
 BPF_CALL_1(get_func_arg_cnt, void *, ctx)
diff --git a/net/core/filter.c b/net/core/filter.c
index ecf2ddf633bf..4be175f84eb9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6346,7 +6346,9 @@ static const struct bpf_func_proto bpf_skb_check_mtu_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1_type      = ARG_PTR_TO_CTX,
 	.arg2_type      = ARG_ANYTHING,
-	.arg3_type      = ARG_PTR_TO_INT,
+	.arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg3_size	= sizeof(u32),
 	.arg4_type      = ARG_ANYTHING,
 	.arg5_type      = ARG_ANYTHING,
 };
@@ -6357,7 +6359,9 @@ static const struct bpf_func_proto bpf_xdp_check_mtu_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1_type      = ARG_PTR_TO_CTX,
 	.arg2_type      = ARG_ANYTHING,
-	.arg3_type      = ARG_PTR_TO_INT,
+	.arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg3_size	= sizeof(u32),
 	.arg4_type      = ARG_ANYTHING,
 	.arg5_type      = ARG_ANYTHING,
 };
-- 
2.43.0

Re: [PATCH bpf-next v4 3/8] bpf: Fix helper writes to read-only maps

[Andrii Nakryiko]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Lonial found an issue that despite user- and BPF-side frozen BPF map
> (like in case of .rodata), it was still possible to write into it from
> a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
> as arguments.
>
> In check_func_arg() when the argument is as mentioned, the meta->raw_mode
> is never set. Later, check_helper_mem_access(), under the case of
> PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
> subsequent call to check_map_access_type() and given the BPF map is
> read-only it succeeds.
>
> The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> when results are written into them as opposed to read out of them. The
> latter indicates that it's okay to pass a pointer to uninitialized memory
> as the memory is written to anyway.
>
> However, ARG_PTR_TO_{LONG,INT} is a special case of ARG_PTR_TO_FIXED_SIZE_MEM
> just with additional alignment requirement. So it is better to just get
> rid of the ARG_PTR_TO_{LONG,INT} special cases altogether and reuse the
> fixed size memory types. For this, add MEM_ALIGNED to additionally ensure
> alignment given these helpers write directly into the args via *<ptr> = val.
> The .arg*_size has been initialized reflecting the actual sizeof(*<ptr>).
>
> MEM_ALIGNED can only be used in combination with MEM_FIXED_SIZE annotated
> argument types, since in !MEM_FIXED_SIZE cases the verifier does not know
> the buffer size a priori and therefore cannot blindly write *<ptr> = val.
>
> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> Reported-by: Lonial Con <kongln9170@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  v1 -> v2:
>  - switch to MEM_FIXED_SIZE
>  v3 -> v4:
>  - fixed 64bit in strto{u,}l (Alexei)
>
>  include/linux/bpf.h      |  7 +++++--
>  kernel/bpf/helpers.c     |  8 ++++++--
>  kernel/bpf/syscall.c     |  4 +++-
>  kernel/bpf/verifier.c    | 38 +++++---------------------------------
>  kernel/trace/bpf_trace.c |  8 ++++++--
>  net/core/filter.c        |  8 ++++++--
>  6 files changed, 31 insertions(+), 42 deletions(-)
>

Very neat. I only have stylistic nits, but LGTM anyways

Acked-by: Andrii Nakryiko <andrii@kernel.org>

[...]

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 5404bb964d83..0587d0c2375a 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -537,7 +537,9 @@ const struct bpf_func_proto bpf_strtol_proto = {
>         .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
>         .arg2_type      = ARG_CONST_SIZE,
>         .arg3_type      = ARG_ANYTHING,
> -       .arg4_type      = ARG_PTR_TO_LONG,
> +       .arg4_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
> +                         MEM_UNINIT | MEM_ALIGNED,

nit: I wouldn't wrap the line here and everywhere else

> +       .arg4_size      = sizeof(s64),
>  };
>
>  BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> @@ -563,7 +565,9 @@ const struct bpf_func_proto bpf_strtoul_proto = {
>         .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
>         .arg2_type      = ARG_CONST_SIZE,
>         .arg3_type      = ARG_ANYTHING,
> -       .arg4_type      = ARG_PTR_TO_LONG,
> +       .arg4_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
> +                         MEM_UNINIT | MEM_ALIGNED,
> +       .arg4_size      = sizeof(u64),
>  };
>

[...]

>  static const struct bpf_reg_types spin_lock_types = {
>         .types = {
>                 PTR_TO_MAP_VALUE,
> @@ -8453,8 +8433,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>         [ARG_PTR_TO_SPIN_LOCK]          = &spin_lock_types,
>         [ARG_PTR_TO_MEM]                = &mem_types,
>         [ARG_PTR_TO_RINGBUF_MEM]        = &ringbuf_mem_types,
> -       [ARG_PTR_TO_INT]                = &int_ptr_types,
> -       [ARG_PTR_TO_LONG]               = &int_ptr_types,
>         [ARG_PTR_TO_PERCPU_BTF_ID]      = &percpu_btf_ptr_types,
>         [ARG_PTR_TO_FUNC]               = &func_ptr_types,
>         [ARG_PTR_TO_STACK]              = &stack_ptr_types,
> @@ -9020,6 +8998,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                         err = check_helper_mem_access(env, regno,
>                                                       fn->arg_size[arg], false,
>                                                       meta);
> +                       if (err)
> +                               return err;
> +                       if (arg_type & MEM_ALIGNED)
> +                               err = check_ptr_alignment(env, reg, 0,
> +                                                         fn->arg_size[arg], true);

nit: we should take advantage of 100 character lines and make this streamlined:

@@ -9016,11 +9016,10 @@ static int check_func_arg(struct
bpf_verifier_env *env, u32 arg,
                 * next is_mem_size argument below.
                 */
                meta->raw_mode = arg_type & MEM_UNINIT;
-               if (arg_type & MEM_FIXED_SIZE) {
-                       err = check_helper_mem_access(env, regno,
-                                                     fn->arg_size[arg], false,
-                                                     meta);
-               }
+               if (arg_type & MEM_FIXED_SIZE)
+                       err = check_helper_mem_access(env, regno,
fn->arg_size[arg], false, meta);
+               if (arg_type & MEM_ALIGNED)
+                       err = err ?: check_ptr_alignment(env, reg, 0,
fn->arg_size[arg], true);
                break;

>                 }
>                 break;
>         case ARG_CONST_SIZE:
> @@ -9044,17 +9027,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                 if (err)
>                         return err;
>                 break;

[...]

Re: [PATCH bpf-next v4 3/8] bpf: Fix helper writes to read-only maps

[Alexei Starovoitov]

On Fri, Sep 6, 2024 at 2:03 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > Lonial found an issue that despite user- and BPF-side frozen BPF map
> > (like in case of .rodata), it was still possible to write into it from
> > a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
> > as arguments.
> >
> > In check_func_arg() when the argument is as mentioned, the meta->raw_mode
> > is never set. Later, check_helper_mem_access(), under the case of
> > PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
> > subsequent call to check_map_access_type() and given the BPF map is
> > read-only it succeeds.
> >
> > The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> > when results are written into them as opposed to read out of them. The
> > latter indicates that it's okay to pass a pointer to uninitialized memory
> > as the memory is written to anyway.
> >
> > However, ARG_PTR_TO_{LONG,INT} is a special case of ARG_PTR_TO_FIXED_SIZE_MEM
> > just with additional alignment requirement. So it is better to just get
> > rid of the ARG_PTR_TO_{LONG,INT} special cases altogether and reuse the
> > fixed size memory types. For this, add MEM_ALIGNED to additionally ensure
> > alignment given these helpers write directly into the args via *<ptr> = val.
> > The .arg*_size has been initialized reflecting the actual sizeof(*<ptr>).
> >
> > MEM_ALIGNED can only be used in combination with MEM_FIXED_SIZE annotated
> > argument types, since in !MEM_FIXED_SIZE cases the verifier does not know
> > the buffer size a priori and therefore cannot blindly write *<ptr> = val.
> >
> > Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> > Reported-by: Lonial Con <kongln9170@gmail.com>
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > ---
> >  v1 -> v2:
> >  - switch to MEM_FIXED_SIZE
> >  v3 -> v4:
> >  - fixed 64bit in strto{u,}l (Alexei)
> >
> >  include/linux/bpf.h      |  7 +++++--
> >  kernel/bpf/helpers.c     |  8 ++++++--
> >  kernel/bpf/syscall.c     |  4 +++-
> >  kernel/bpf/verifier.c    | 38 +++++---------------------------------
> >  kernel/trace/bpf_trace.c |  8 ++++++--
> >  net/core/filter.c        |  8 ++++++--
> >  6 files changed, 31 insertions(+), 42 deletions(-)
> >
>
> Very neat. I only have stylistic nits, but LGTM anyways
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> [...]
>
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 5404bb964d83..0587d0c2375a 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -537,7 +537,9 @@ const struct bpf_func_proto bpf_strtol_proto = {
> >         .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
> >         .arg2_type      = ARG_CONST_SIZE,
> >         .arg3_type      = ARG_ANYTHING,
> > -       .arg4_type      = ARG_PTR_TO_LONG,
> > +       .arg4_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
> > +                         MEM_UNINIT | MEM_ALIGNED,
>
> nit: I wouldn't wrap the line here and everywhere else
>
> > +       .arg4_size      = sizeof(s64),
> >  };
> >
> >  BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> > @@ -563,7 +565,9 @@ const struct bpf_func_proto bpf_strtoul_proto = {
> >         .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
> >         .arg2_type      = ARG_CONST_SIZE,
> >         .arg3_type      = ARG_ANYTHING,
> > -       .arg4_type      = ARG_PTR_TO_LONG,
> > +       .arg4_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
> > +                         MEM_UNINIT | MEM_ALIGNED,
> > +       .arg4_size      = sizeof(u64),
> >  };
> >
>
> [...]
>
> >  static const struct bpf_reg_types spin_lock_types = {
> >         .types = {
> >                 PTR_TO_MAP_VALUE,
> > @@ -8453,8 +8433,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
> >         [ARG_PTR_TO_SPIN_LOCK]          = &spin_lock_types,
> >         [ARG_PTR_TO_MEM]                = &mem_types,
> >         [ARG_PTR_TO_RINGBUF_MEM]        = &ringbuf_mem_types,
> > -       [ARG_PTR_TO_INT]                = &int_ptr_types,
> > -       [ARG_PTR_TO_LONG]               = &int_ptr_types,
> >         [ARG_PTR_TO_PERCPU_BTF_ID]      = &percpu_btf_ptr_types,
> >         [ARG_PTR_TO_FUNC]               = &func_ptr_types,
> >         [ARG_PTR_TO_STACK]              = &stack_ptr_types,
> > @@ -9020,6 +8998,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >                         err = check_helper_mem_access(env, regno,
> >                                                       fn->arg_size[arg], false,
> >                                                       meta);
> > +                       if (err)
> > +                               return err;
> > +                       if (arg_type & MEM_ALIGNED)
> > +                               err = check_ptr_alignment(env, reg, 0,
> > +                                                         fn->arg_size[arg], true);
>
> nit: we should take advantage of 100 character lines and make this streamlined:
>
> @@ -9016,11 +9016,10 @@ static int check_func_arg(struct
> bpf_verifier_env *env, u32 arg,
>                  * next is_mem_size argument below.
>                  */
>                 meta->raw_mode = arg_type & MEM_UNINIT;
> -               if (arg_type & MEM_FIXED_SIZE) {
> -                       err = check_helper_mem_access(env, regno,
> -                                                     fn->arg_size[arg], false,
> -                                                     meta);
> -               }
> +               if (arg_type & MEM_FIXED_SIZE)
> +                       err = check_helper_mem_access(env, regno,
> fn->arg_size[arg], false, meta);
> +               if (arg_type & MEM_ALIGNED)
> +                       err = err ?: check_ptr_alignment(env, reg, 0,
> fn->arg_size[arg], true);
>                 break;

Agree that it's a bit cleaner this way.

Re: [PATCH bpf-next v4 3/8] bpf: Fix helper writes to read-only maps

[Shung-Hsi Yu]

On Fri, Sep 06, 2024 at 03:56:03PM GMT, Daniel Borkmann wrote:
> Lonial found an issue that despite user- and BPF-side frozen BPF map
> (like in case of .rodata), it was still possible to write into it from
> a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
> as arguments.
> 
> In check_func_arg() when the argument is as mentioned, the meta->raw_mode
> is never set. Later, check_helper_mem_access(), under the case of
> PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
> subsequent call to check_map_access_type() and given the BPF map is
> read-only it succeeds.
> 
> The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
> when results are written into them as opposed to read out of them. The
> latter indicates that it's okay to pass a pointer to uninitialized memory
> as the memory is written to anyway.
> 
> However, ARG_PTR_TO_{LONG,INT} is a special case of ARG_PTR_TO_FIXED_SIZE_MEM
> just with additional alignment requirement. So it is better to just get
> rid of the ARG_PTR_TO_{LONG,INT} special cases altogether and reuse the
> fixed size memory types. For this, add MEM_ALIGNED to additionally ensure
> alignment given these helpers write directly into the args via *<ptr> = val.
> The .arg*_size has been initialized reflecting the actual sizeof(*<ptr>).
> 
> MEM_ALIGNED can only be used in combination with MEM_FIXED_SIZE annotated
> argument types, since in !MEM_FIXED_SIZE cases the verifier does not know
> the buffer size a priori and therefore cannot blindly write *<ptr> = val.
> 
> Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
> Reported-by: Lonial Con <kongln9170@gmail.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Agree with Andrii's suggestion in silbing thread. That said, logic-wise LGTM

Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>

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

[Daniel Borkmann]

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 <shung-hsi.yu@suse.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 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(-)

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

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

[Andrii Nakryiko]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> 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 <shung-hsi.yu@suse.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  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 <andrii@kernel.org>


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

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

[Shung-Hsi Yu]

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 <shung-hsi.yu@suse.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>

[PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Daniel Borkmann]

For all non-tracing helpers which formerly had ARG_PTR_TO_{LONG,INT} as input
arguments, zero the value for the case of an error as otherwise it could leak
memory. For tracing, it is not needed given CAP_PERFMON can already read all
kernel memory anyway hence bpf_get_func_arg() and bpf_get_func_ret() is skipped
in here.

Also, rearrange the MTU checker helpers a bit to among other nit fixes
consolidate flag checks such that we only need to zero in one location with
regards to malformed flag inputs.

Fixes: 8a67f2de9b1d ("bpf: expose bpf_strtol and bpf_strtoul to all program types")
Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 v1 -> v2:
 - only set *mtu_len in error path (Alexei)

 kernel/bpf/helpers.c |  2 ++
 kernel/bpf/syscall.c |  1 +
 net/core/filter.c    | 35 +++++++++++++++++------------------
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 0587d0c2375a..ff66a0522799 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -523,6 +523,7 @@ BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
 	long long _res;
 	int err;
 
+	*res = 0;
 	err = __bpf_strtoll(buf, buf_len, flags, &_res);
 	if (err < 0)
 		return err;
@@ -549,6 +550,7 @@ BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
 	bool is_negative;
 	int err;
 
+	*res = 0;
 	err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative);
 	if (err < 0)
 		return err;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index feb276771c03..513b4301a0af 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5934,6 +5934,7 @@ static const struct bpf_func_proto bpf_sys_close_proto = {
 
 BPF_CALL_4(bpf_kallsyms_lookup_name, const char *, name, int, name_sz, int, flags, u64 *, res)
 {
+	*res = 0;
 	if (flags)
 		return -EINVAL;
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 4be175f84eb9..c219385e7bb4 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6264,18 +6264,19 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb,
 	int skb_len, dev_len;
 	int mtu;
 
-	if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
-		return -EINVAL;
-
-	if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
+	if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
+		     (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
+		*mtu_len = 0;
 		return -EINVAL;
+	}
 
 	dev = __dev_via_ifindex(dev, ifindex);
-	if (unlikely(!dev))
+	if (unlikely(!dev)) {
+		*mtu_len = 0;
 		return -ENODEV;
+	}
 
 	mtu = READ_ONCE(dev->mtu);
-
 	dev_len = mtu + dev->hard_header_len;
 
 	/* If set use *mtu_len as input, L3 as iph->tot_len (like fib_lookup) */
@@ -6286,10 +6287,10 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb,
 		ret = BPF_MTU_CHK_RET_SUCCESS;
 		goto out;
 	}
-	/* At this point, skb->len exceed MTU, but as it include length of all
-	 * segments, it can still be below MTU.  The SKB can possibly get
-	 * re-segmented in transmit path (see validate_xmit_skb).  Thus, user
-	 * must choose if segs are to be MTU checked.
+	/* At this point, skb->len exceeds MTU, but as it includes the length
+	 * of all segments, it can still be below MTU. The skb can possibly
+	 * get re-segmented in transmit path (see validate_xmit_skb). Thus,
+	 * the user must choose if segments are to be MTU checked.
 	 */
 	if (skb_is_gso(skb)) {
 		ret = BPF_MTU_CHK_RET_SUCCESS;
@@ -6299,9 +6300,7 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb,
 			ret = BPF_MTU_CHK_RET_SEGS_TOOBIG;
 	}
 out:
-	/* BPF verifier guarantees valid pointer */
 	*mtu_len = mtu;
-
 	return ret;
 }
 
@@ -6314,16 +6313,18 @@ BPF_CALL_5(bpf_xdp_check_mtu, struct xdp_buff *, xdp,
 	int mtu, dev_len;
 
 	/* XDP variant doesn't support multi-buffer segment check (yet) */
-	if (unlikely(flags))
+	if (unlikely(flags)) {
+		*mtu_len = 0;
 		return -EINVAL;
+	}
 
 	dev = __dev_via_ifindex(dev, ifindex);
-	if (unlikely(!dev))
+	if (unlikely(!dev)) {
+		*mtu_len = 0;
 		return -ENODEV;
+	}
 
 	mtu = READ_ONCE(dev->mtu);
-
-	/* Add L2-header as dev MTU is L3 size */
 	dev_len = mtu + dev->hard_header_len;
 
 	/* Use *mtu_len as input, L3 as iph->tot_len (like fib_lookup) */
@@ -6334,9 +6335,7 @@ BPF_CALL_5(bpf_xdp_check_mtu, struct xdp_buff *, xdp,
 	if (xdp_len > dev_len)
 		ret = BPF_MTU_CHK_RET_FRAG_NEEDED;
 
-	/* BPF verifier guarantees valid pointer */
 	*mtu_len = mtu;
-
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Andrii Nakryiko]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> For all non-tracing helpers which formerly had ARG_PTR_TO_{LONG,INT} as input
> arguments, zero the value for the case of an error as otherwise it could leak
> memory. For tracing, it is not needed given CAP_PERFMON can already read all
> kernel memory anyway hence bpf_get_func_arg() and bpf_get_func_ret() is skipped
> in here.
>
> Also, rearrange the MTU checker helpers a bit to among other nit fixes
> consolidate flag checks such that we only need to zero in one location with
> regards to malformed flag inputs.
>
> Fixes: 8a67f2de9b1d ("bpf: expose bpf_strtol and bpf_strtoul to all program types")
> Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  v1 -> v2:
>  - only set *mtu_len in error path (Alexei)
>
>  kernel/bpf/helpers.c |  2 ++
>  kernel/bpf/syscall.c |  1 +
>  net/core/filter.c    | 35 +++++++++++++++++------------------
>  3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 0587d0c2375a..ff66a0522799 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -523,6 +523,7 @@ BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
>         long long _res;
>         int err;
>
> +       *res = 0;
>         err = __bpf_strtoll(buf, buf_len, flags, &_res);
>         if (err < 0)
>                 return err;
> @@ -549,6 +550,7 @@ BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
>         bool is_negative;
>         int err;
>
> +       *res = 0;
>         err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative);
>         if (err < 0)
>                 return err;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index feb276771c03..513b4301a0af 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5934,6 +5934,7 @@ static const struct bpf_func_proto bpf_sys_close_proto = {
>
>  BPF_CALL_4(bpf_kallsyms_lookup_name, const char *, name, int, name_sz, int, flags, u64 *, res)
>  {
> +       *res = 0;
>         if (flags)
>                 return -EINVAL;
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 4be175f84eb9..c219385e7bb4 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6264,18 +6264,19 @@ BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb,
>         int skb_len, dev_len;
>         int mtu;
>
> -       if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
> -               return -EINVAL;
> -
> -       if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
> +       if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
> +                    (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
> +               *mtu_len = 0;
>                 return -EINVAL;
> +       }

meh, why? you have *mtu_len = 0 below anyways, so there is already
duplication. I'd rather have extra *mtu_len than much more convoluted
condition

>
>         dev = __dev_via_ifindex(dev, ifindex);
> -       if (unlikely(!dev))
> +       if (unlikely(!dev)) {
> +               *mtu_len = 0;
>                 return -ENODEV;
> +       }
>
>         mtu = READ_ONCE(dev->mtu);
> -
>         dev_len = mtu + dev->hard_header_len;
>
>         /* If set use *mtu_len as input, L3 as iph->tot_len (like fib_lookup) */

[...]

Re: [PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Alexei Starovoitov]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> -       if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
> -               return -EINVAL;
> -
> -       if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
> +       if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
> +                    (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
> +               *mtu_len = 0;
>                 return -EINVAL;
> +       }
>
>         dev = __dev_via_ifindex(dev, ifindex);
> -       if (unlikely(!dev))
> +       if (unlikely(!dev)) {
> +               *mtu_len = 0;
>                 return -ENODEV;
> +       }

I don't understand this mtu_len clearing.

My earlier comment was that mtu is in&out argument.
The program has to set it to something. It cannot be uninit.
So zeroing it on error looks very odd.

In that sense the patch 3 looks wrong. Instead of:

@@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
bpf_skb_check_mtu_proto = {
        .ret_type       = RET_INTEGER,
        .arg1_type      = ARG_PTR_TO_CTX,
        .arg2_type      = ARG_ANYTHING,
-       .arg3_type      = ARG_PTR_TO_INT,
+       .arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
+                         MEM_UNINIT | MEM_ALIGNED,
+       .arg3_size      = sizeof(u32),

MEM_UNINIT should be removed, because
bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.

If there is a program out there that calls this helper without
initializing mtu_len it will be rejected after we fix it,
but I think that's a good thing.
Passing random mtu_len and let helper do:

skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;

is just bad.


pw-bot: cr

Re: [PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Daniel Borkmann]

On 9/7/24 12:35 AM, Alexei Starovoitov wrote:
> On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> -       if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
>> -               return -EINVAL;
>> -
>> -       if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
>> +       if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
>> +                    (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
>> +               *mtu_len = 0;
>>                  return -EINVAL;
>> +       }
>>
>>          dev = __dev_via_ifindex(dev, ifindex);
>> -       if (unlikely(!dev))
>> +       if (unlikely(!dev)) {
>> +               *mtu_len = 0;
>>                  return -ENODEV;
>> +       }
> 
> I don't understand this mtu_len clearing.
> 
> My earlier comment was that mtu is in&out argument.
> The program has to set it to something. It cannot be uninit.
> So zeroing it on error looks very odd.
> 
> In that sense the patch 3 looks wrong. Instead of:
> 
> @@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
> bpf_skb_check_mtu_proto = {
>          .ret_type       = RET_INTEGER,
>          .arg1_type      = ARG_PTR_TO_CTX,
>          .arg2_type      = ARG_ANYTHING,
> -       .arg3_type      = ARG_PTR_TO_INT,
> +       .arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
> +                         MEM_UNINIT | MEM_ALIGNED,
> +       .arg3_size      = sizeof(u32),
> 
> MEM_UNINIT should be removed, because
> bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.
> 
> If there is a program out there that calls this helper without
> initializing mtu_len it will be rejected after we fix it,
> but I think that's a good thing.
> Passing random mtu_len and let helper do:
> 
> skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;
> 
> is just bad.

Ok, fair. Removing MEM_UNINIT sounds reasonable, was mostly thinking
that even if its garbage MTU being passed in, mtu_len gets filled in
either case (BPF_MTU_CHK_RET_{SUCCESS,FRAG_NEEDED}) assuming no invalid
ifindex e.g.:

   __u32 mtu_len;
   bpf_skb_check_mtu(skb, skb->ifindex, &mtu_len, 0, 0);

Will spin a v5 then.

Thanks,
Daniel

Re: [PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Daniel Borkmann]

On 9/9/24 2:16 PM, Daniel Borkmann wrote:
> On 9/7/24 12:35 AM, Alexei Starovoitov wrote:
>> On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>
>>> -       if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
>>> -               return -EINVAL;
>>> -
>>> -       if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
>>> +       if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
>>> +                    (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
>>> +               *mtu_len = 0;
>>>                  return -EINVAL;
>>> +       }
>>>
>>>          dev = __dev_via_ifindex(dev, ifindex);
>>> -       if (unlikely(!dev))
>>> +       if (unlikely(!dev)) {
>>> +               *mtu_len = 0;
>>>                  return -ENODEV;
>>> +       }
>>
>> I don't understand this mtu_len clearing.
>>
>> My earlier comment was that mtu is in&out argument.
>> The program has to set it to something. It cannot be uninit.
>> So zeroing it on error looks very odd.
>>
>> In that sense the patch 3 looks wrong. Instead of:
>>
>> @@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
>> bpf_skb_check_mtu_proto = {
>>          .ret_type       = RET_INTEGER,
>>          .arg1_type      = ARG_PTR_TO_CTX,
>>          .arg2_type      = ARG_ANYTHING,
>> -       .arg3_type      = ARG_PTR_TO_INT,
>> +       .arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
>> +                         MEM_UNINIT | MEM_ALIGNED,
>> +       .arg3_size      = sizeof(u32),
>>
>> MEM_UNINIT should be removed, because
>> bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.
>>
>> If there is a program out there that calls this helper without
>> initializing mtu_len it will be rejected after we fix it,
>> but I think that's a good thing.
>> Passing random mtu_len and let helper do:
>>
>> skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;
>>
>> is just bad.
> 
> Ok, fair. Removing MEM_UNINIT sounds reasonable, was mostly thinking
> that even if its garbage MTU being passed in, mtu_len gets filled in
> either case (BPF_MTU_CHK_RET_{SUCCESS,FRAG_NEEDED}) assuming no invalid
> ifindex e.g.:
> 
>    __u32 mtu_len;
>    bpf_skb_check_mtu(skb, skb->ifindex, &mtu_len, 0, 0);

Getting back at this, removing MEM_UNINIT needs more verifier rework:
MEM_UNINIT right now implies two things actually: i) write into memory,
ii) memory does not have to be initialized. If we lift MEM_UNINIT, it
then becomes: i) read into memory, ii) memory must be initialized.

This means that for bpf_*_check_mtu() we're readding the issue we're
trying to fix, that is, it would then be able to write back into things
like .rodata maps.

My suggestion is for this series is to go with MEM_UNINIT tag and
clearing on error case to avoid leaking, and then in a subsequent
series to break up MEM_UNINIT in the verifier into two properties:
MEM_WRITE and MEM_UNINIT to better declare intent of the helpers. Then
the bpf_*_check_mtu() will have MEM_WRITE but not MEM_UNINIT.

Thoughts? (If preference is to further extend this series, I can also
look into that ofc.)

Thanks,
Daniel

Re: [PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Andrii Nakryiko]

On Thu, Sep 12, 2024 at 2:47 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/9/24 2:16 PM, Daniel Borkmann wrote:
> > On 9/7/24 12:35 AM, Alexei Starovoitov wrote:
> >> On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>
> >>> -       if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
> >>> -               return -EINVAL;
> >>> -
> >>> -       if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
> >>> +       if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
> >>> +                    (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
> >>> +               *mtu_len = 0;
> >>>                  return -EINVAL;
> >>> +       }
> >>>
> >>>          dev = __dev_via_ifindex(dev, ifindex);
> >>> -       if (unlikely(!dev))
> >>> +       if (unlikely(!dev)) {
> >>> +               *mtu_len = 0;
> >>>                  return -ENODEV;
> >>> +       }
> >>
> >> I don't understand this mtu_len clearing.
> >>
> >> My earlier comment was that mtu is in&out argument.
> >> The program has to set it to something. It cannot be uninit.
> >> So zeroing it on error looks very odd.
> >>
> >> In that sense the patch 3 looks wrong. Instead of:
> >>
> >> @@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
> >> bpf_skb_check_mtu_proto = {
> >>          .ret_type       = RET_INTEGER,
> >>          .arg1_type      = ARG_PTR_TO_CTX,
> >>          .arg2_type      = ARG_ANYTHING,
> >> -       .arg3_type      = ARG_PTR_TO_INT,
> >> +       .arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
> >> +                         MEM_UNINIT | MEM_ALIGNED,
> >> +       .arg3_size      = sizeof(u32),
> >>
> >> MEM_UNINIT should be removed, because
> >> bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.
> >>
> >> If there is a program out there that calls this helper without
> >> initializing mtu_len it will be rejected after we fix it,
> >> but I think that's a good thing.
> >> Passing random mtu_len and let helper do:
> >>
> >> skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;
> >>
> >> is just bad.
> >
> > Ok, fair. Removing MEM_UNINIT sounds reasonable, was mostly thinking
> > that even if its garbage MTU being passed in, mtu_len gets filled in
> > either case (BPF_MTU_CHK_RET_{SUCCESS,FRAG_NEEDED}) assuming no invalid
> > ifindex e.g.:
> >
> >    __u32 mtu_len;
> >    bpf_skb_check_mtu(skb, skb->ifindex, &mtu_len, 0, 0);
>
> Getting back at this, removing MEM_UNINIT needs more verifier rework:
> MEM_UNINIT right now implies two things actually: i) write into memory,
> ii) memory does not have to be initialized. If we lift MEM_UNINIT, it
> then becomes: i) read into memory, ii) memory must be initialized.
>
> This means that for bpf_*_check_mtu() we're readding the issue we're
> trying to fix, that is, it would then be able to write back into things
> like .rodata maps.
>
> My suggestion is for this series is to go with MEM_UNINIT tag and
> clearing on error case to avoid leaking, and then in a subsequent
> series to break up MEM_UNINIT in the verifier into two properties:
> MEM_WRITE and MEM_UNINIT to better declare intent of the helpers. Then
> the bpf_*_check_mtu() will have MEM_WRITE but not MEM_UNINIT.
>
> Thoughts? (If preference is to further extend this series, I can also
> look into that ofc.)

We have MEM_RDONLY which literally means that memory is meant to be
only consumed for reading. Will this solve these issues?

>
> Thanks,
> Daniel

Re: [PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Daniel Borkmann]

On 9/12/24 7:59 PM, Andrii Nakryiko wrote:
> On Thu, Sep 12, 2024 at 2:47 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 9/9/24 2:16 PM, Daniel Borkmann wrote:
>>> On 9/7/24 12:35 AM, Alexei Starovoitov wrote:
>>>> On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>>
>>>>> -       if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
>>>>> -               return -EINVAL;
>>>>> -
>>>>> -       if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
>>>>> +       if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
>>>>> +                    (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
>>>>> +               *mtu_len = 0;
>>>>>                   return -EINVAL;
>>>>> +       }
>>>>>
>>>>>           dev = __dev_via_ifindex(dev, ifindex);
>>>>> -       if (unlikely(!dev))
>>>>> +       if (unlikely(!dev)) {
>>>>> +               *mtu_len = 0;
>>>>>                   return -ENODEV;
>>>>> +       }
>>>>
>>>> I don't understand this mtu_len clearing.
>>>>
>>>> My earlier comment was that mtu is in&out argument.
>>>> The program has to set it to something. It cannot be uninit.
>>>> So zeroing it on error looks very odd.
>>>>
>>>> In that sense the patch 3 looks wrong. Instead of:
>>>>
>>>> @@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
>>>> bpf_skb_check_mtu_proto = {
>>>>           .ret_type       = RET_INTEGER,
>>>>           .arg1_type      = ARG_PTR_TO_CTX,
>>>>           .arg2_type      = ARG_ANYTHING,
>>>> -       .arg3_type      = ARG_PTR_TO_INT,
>>>> +       .arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
>>>> +                         MEM_UNINIT | MEM_ALIGNED,
>>>> +       .arg3_size      = sizeof(u32),
>>>>
>>>> MEM_UNINIT should be removed, because
>>>> bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.
>>>>
>>>> If there is a program out there that calls this helper without
>>>> initializing mtu_len it will be rejected after we fix it,
>>>> but I think that's a good thing.
>>>> Passing random mtu_len and let helper do:
>>>>
>>>> skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;
>>>>
>>>> is just bad.
>>>
>>> Ok, fair. Removing MEM_UNINIT sounds reasonable, was mostly thinking
>>> that even if its garbage MTU being passed in, mtu_len gets filled in
>>> either case (BPF_MTU_CHK_RET_{SUCCESS,FRAG_NEEDED}) assuming no invalid
>>> ifindex e.g.:
>>>
>>>     __u32 mtu_len;
>>>     bpf_skb_check_mtu(skb, skb->ifindex, &mtu_len, 0, 0);
>>
>> Getting back at this, removing MEM_UNINIT needs more verifier rework:
>> MEM_UNINIT right now implies two things actually: i) write into memory,
>> ii) memory does not have to be initialized. If we lift MEM_UNINIT, it
>> then becomes: i) read into memory, ii) memory must be initialized.
>>
>> This means that for bpf_*_check_mtu() we're readding the issue we're
>> trying to fix, that is, it would then be able to write back into things
>> like .rodata maps.
>>
>> My suggestion is for this series is to go with MEM_UNINIT tag and
>> clearing on error case to avoid leaking, and then in a subsequent
>> series to break up MEM_UNINIT in the verifier into two properties:
>> MEM_WRITE and MEM_UNINIT to better declare intent of the helpers. Then
>> the bpf_*_check_mtu() will have MEM_WRITE but not MEM_UNINIT.
>>
>> Thoughts? (If preference is to further extend this series, I can also
>> look into that ofc.)
> 
> We have MEM_RDONLY which literally means that memory is meant to be
> only consumed for reading. Will this solve these issues?

It doesn't help, in check_helper_mem_access() we check for writes e.g.
for map values only via BPF_WRITE when meta->raw_mode is set which is
the case for MEM_UNINIT-tagged ARG_PTR_TO_MEM, otherwise BPF_READ is
implied hence my suggestion to break this up into two properties as a
next step.

Thanks,
Daniel

[PATCH bpf-next v4 6/8] selftests/bpf: Fix ARG_PTR_TO_LONG {half-,}uninitialized test

[Daniel Borkmann]

The assumption of 'in privileged mode reads from uninitialized stack locations
are permitted' is not quite correct since the verifier was probing for read
access rather than write access. Both tests need to be annotated as __success
for privileged and unprivileged.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 tools/testing/selftests/bpf/progs/verifier_int_ptr.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
index 9fc3fae5cd83..87206803c025 100644
--- a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
@@ -8,7 +8,6 @@
 SEC("socket")
 __description("ARG_PTR_TO_LONG uninitialized")
 __success
-__failure_unpriv __msg_unpriv("invalid indirect read from stack R4 off -16+0 size 8")
 __naked void arg_ptr_to_long_uninitialized(void)
 {
 	asm volatile ("					\
@@ -36,9 +35,7 @@ __naked void arg_ptr_to_long_uninitialized(void)
 
 SEC("socket")
 __description("ARG_PTR_TO_LONG half-uninitialized")
-/* in privileged mode reads from uninitialized stack locations are permitted */
-__success __failure_unpriv
-__msg_unpriv("invalid indirect read from stack R4 off -16+4 size 8")
+__success
 __retval(0)
 __naked void ptr_to_long_half_uninitialized(void)
 {
-- 
2.43.0

Re: [PATCH bpf-next v4 6/8] selftests/bpf: Fix ARG_PTR_TO_LONG {half-,}uninitialized test

[Andrii Nakryiko]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> The assumption of 'in privileged mode reads from uninitialized stack locations
> are permitted' is not quite correct since the verifier was probing for read
> access rather than write access. Both tests need to be annotated as __success
> for privileged and unprivileged.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  tools/testing/selftests/bpf/progs/verifier_int_ptr.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> diff --git a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
> index 9fc3fae5cd83..87206803c025 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
> @@ -8,7 +8,6 @@
>  SEC("socket")
>  __description("ARG_PTR_TO_LONG uninitialized")
>  __success
> -__failure_unpriv __msg_unpriv("invalid indirect read from stack R4 off -16+0 size 8")
>  __naked void arg_ptr_to_long_uninitialized(void)
>  {
>         asm volatile ("                                 \
> @@ -36,9 +35,7 @@ __naked void arg_ptr_to_long_uninitialized(void)
>
>  SEC("socket")
>  __description("ARG_PTR_TO_LONG half-uninitialized")
> -/* in privileged mode reads from uninitialized stack locations are permitted */
> -__success __failure_unpriv
> -__msg_unpriv("invalid indirect read from stack R4 off -16+4 size 8")
> +__success
>  __retval(0)
>  __naked void ptr_to_long_half_uninitialized(void)
>  {
> --
> 2.43.0
>

[PATCH bpf-next v4 7/8] selftests/bpf: Rename ARG_PTR_TO_LONG test description

[Daniel Borkmann]

Given we got rid of ARG_PTR_TO_LONG, change the test case description to
avoid potential confusion:

  # ./vmtest.sh -- ./test_progs -t verifier_int_ptr
  [...]
  ./test_progs -t verifier_int_ptr
  [    1.610563] bpf_testmod: loading out-of-tree module taints kernel.
  [    1.611049] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel
  #489/1   verifier_int_ptr/arg pointer to long uninitialized:OK
  #489/2   verifier_int_ptr/arg pointer to long half-uninitialized:OK
  #489/3   verifier_int_ptr/arg pointer to long misaligned:OK
  #489/4   verifier_int_ptr/arg pointer to long size < sizeof(long):OK
  #489/5   verifier_int_ptr/arg pointer to long initialized:OK
  #489     verifier_int_ptr:OK
  Summary: 1/5 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 v1 -> v2:
 - new patch

 tools/testing/selftests/bpf/progs/verifier_int_ptr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
index 87206803c025..5f2efb895edb 100644
--- a/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_int_ptr.c
@@ -6,7 +6,7 @@
 #include "bpf_misc.h"
 
 SEC("socket")
-__description("ARG_PTR_TO_LONG uninitialized")
+__description("arg pointer to long uninitialized")
 __success
 __naked void arg_ptr_to_long_uninitialized(void)
 {
@@ -34,7 +34,7 @@ __naked void arg_ptr_to_long_uninitialized(void)
 }
 
 SEC("socket")
-__description("ARG_PTR_TO_LONG half-uninitialized")
+__description("arg pointer to long half-uninitialized")
 __success
 __retval(0)
 __naked void ptr_to_long_half_uninitialized(void)
@@ -64,7 +64,7 @@ __naked void ptr_to_long_half_uninitialized(void)
 }
 
 SEC("cgroup/sysctl")
-__description("ARG_PTR_TO_LONG misaligned")
+__description("arg pointer to long misaligned")
 __failure __msg("misaligned stack access off 0+-20+0 size 8")
 __naked void arg_ptr_to_long_misaligned(void)
 {
@@ -95,7 +95,7 @@ __naked void arg_ptr_to_long_misaligned(void)
 }
 
 SEC("cgroup/sysctl")
-__description("ARG_PTR_TO_LONG size < sizeof(long)")
+__description("arg pointer to long size < sizeof(long)")
 __failure __msg("invalid indirect access to stack R4 off=-4 size=8")
 __naked void to_long_size_sizeof_long(void)
 {
@@ -124,7 +124,7 @@ __naked void to_long_size_sizeof_long(void)
 }
 
 SEC("cgroup/sysctl")
-__description("ARG_PTR_TO_LONG initialized")
+__description("arg pointer to long initialized")
 __success
 __naked void arg_ptr_to_long_initialized(void)
 {
-- 
2.43.0

[PATCH bpf-next v4 8/8] selftests/bpf: Add a test case to write into .rodata

[Daniel Borkmann]

Add a test case which attempts to write into .rodata section of the
BPF program, and for comparison this adds test cases also for .bss
and .data section.

Before fix:

  # ./vmtest.sh -- ./test_progs -t verifier_const
  [...]
  ./test_progs -t verifier_const
  tester_init:PASS:tester_log_buf 0 nsec
  process_subtest:PASS:obj_open_mem 0 nsec
  process_subtest:PASS:specs_alloc 0 nsec
  run_subtest:PASS:obj_open_mem 0 nsec
  run_subtest:FAIL:unexpected_load_success unexpected success: 0
  #465/1   verifier_const/rodata: write rejected:FAIL
  #465/2   verifier_const/bss: write accepted:OK
  #465/3   verifier_const/data: write accepted:OK
  #465     verifier_const:FAIL
  [...]

After fix:

  # ./vmtest.sh -- ./test_progs -t verifier_const
  [...]
  ./test_progs -t verifier_const
  #465/1   verifier_const/rodata: write rejected:OK
  #465/2   verifier_const/bss: write accepted:OK
  #465/3   verifier_const/data: write accepted:OK
  #465     verifier_const:OK
  [...]

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
---
 v1 -> v2:
 - const volatile long (Andrii)

 .../selftests/bpf/prog_tests/verifier.c       |  2 +
 .../selftests/bpf/progs/verifier_const.c      | 42 +++++++++++++++++++
 2 files changed, 44 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_const.c

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index df398e714dff..e26b5150fc43 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -21,6 +21,7 @@
 #include "verifier_cgroup_inv_retcode.skel.h"
 #include "verifier_cgroup_skb.skel.h"
 #include "verifier_cgroup_storage.skel.h"
+#include "verifier_const.skel.h"
 #include "verifier_const_or.skel.h"
 #include "verifier_ctx.skel.h"
 #include "verifier_ctx_sk_msg.skel.h"
@@ -146,6 +147,7 @@ void test_verifier_cfg(void)                  { RUN(verifier_cfg); }
 void test_verifier_cgroup_inv_retcode(void)   { RUN(verifier_cgroup_inv_retcode); }
 void test_verifier_cgroup_skb(void)           { RUN(verifier_cgroup_skb); }
 void test_verifier_cgroup_storage(void)       { RUN(verifier_cgroup_storage); }
+void test_verifier_const(void)                { RUN(verifier_const); }
 void test_verifier_const_or(void)             { RUN(verifier_const_or); }
 void test_verifier_ctx(void)                  { RUN(verifier_ctx); }
 void test_verifier_ctx_sk_msg(void)           { RUN(verifier_ctx_sk_msg); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_const.c b/tools/testing/selftests/bpf/progs/verifier_const.c
new file mode 100644
index 000000000000..5158dbea8c43
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_const.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Isovalent */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+const volatile long foo = 42;
+long bar;
+long bart = 96;
+
+SEC("tc/ingress")
+__description("rodata: write rejected")
+__failure __msg("write into map forbidden")
+int tcx1(struct __sk_buff *skb)
+{
+	char buff[] = { '8', '4', '\0' };
+	bpf_strtol(buff, sizeof(buff), 0, (long *)&foo);
+	return TCX_PASS;
+}
+
+SEC("tc/ingress")
+__description("bss: write accepted")
+__success
+int tcx2(struct __sk_buff *skb)
+{
+	char buff[] = { '8', '4', '\0' };
+	bpf_strtol(buff, sizeof(buff), 0, &bar);
+	return TCX_PASS;
+}
+
+SEC("tc/ingress")
+__description("data: write accepted")
+__success
+int tcx3(struct __sk_buff *skb)
+{
+	char buff[] = { '8', '4', '\0' };
+	bpf_strtol(buff, sizeof(buff), 0, &bart);
+	return TCX_PASS;
+}
+
+char LICENSE[] SEC("license") = "GPL";
-- 
2.43.0

Re: [PATCH bpf-next v4 8/8] selftests/bpf: Add a test case to write into .rodata

[Andrii Nakryiko]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Add a test case which attempts to write into .rodata section of the
> BPF program, and for comparison this adds test cases also for .bss
> and .data section.
>
> Before fix:
>
>   # ./vmtest.sh -- ./test_progs -t verifier_const
>   [...]
>   ./test_progs -t verifier_const
>   tester_init:PASS:tester_log_buf 0 nsec
>   process_subtest:PASS:obj_open_mem 0 nsec
>   process_subtest:PASS:specs_alloc 0 nsec
>   run_subtest:PASS:obj_open_mem 0 nsec
>   run_subtest:FAIL:unexpected_load_success unexpected success: 0
>   #465/1   verifier_const/rodata: write rejected:FAIL
>   #465/2   verifier_const/bss: write accepted:OK
>   #465/3   verifier_const/data: write accepted:OK
>   #465     verifier_const:FAIL
>   [...]
>
> After fix:
>
>   # ./vmtest.sh -- ./test_progs -t verifier_const
>   [...]
>   ./test_progs -t verifier_const
>   #465/1   verifier_const/rodata: write rejected:OK
>   #465/2   verifier_const/bss: write accepted:OK
>   #465/3   verifier_const/data: write accepted:OK
>   #465     verifier_const:OK
>   [...]
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
> ---
>  v1 -> v2:
>  - const volatile long (Andrii)
>

LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  .../selftests/bpf/prog_tests/verifier.c       |  2 +
>  .../selftests/bpf/progs/verifier_const.c      | 42 +++++++++++++++++++
>  2 files changed, 44 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/verifier_const.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index df398e714dff..e26b5150fc43 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -21,6 +21,7 @@
>  #include "verifier_cgroup_inv_retcode.skel.h"
>  #include "verifier_cgroup_skb.skel.h"
>  #include "verifier_cgroup_storage.skel.h"
> +#include "verifier_const.skel.h"
>  #include "verifier_const_or.skel.h"
>  #include "verifier_ctx.skel.h"
>  #include "verifier_ctx_sk_msg.skel.h"
> @@ -146,6 +147,7 @@ void test_verifier_cfg(void)                  { RUN(verifier_cfg); }
>  void test_verifier_cgroup_inv_retcode(void)   { RUN(verifier_cgroup_inv_retcode); }
>  void test_verifier_cgroup_skb(void)           { RUN(verifier_cgroup_skb); }
>  void test_verifier_cgroup_storage(void)       { RUN(verifier_cgroup_storage); }
> +void test_verifier_const(void)                { RUN(verifier_const); }
>  void test_verifier_const_or(void)             { RUN(verifier_const_or); }
>  void test_verifier_ctx(void)                  { RUN(verifier_ctx); }
>  void test_verifier_ctx_sk_msg(void)           { RUN(verifier_ctx_sk_msg); }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_const.c b/tools/testing/selftests/bpf/progs/verifier_const.c
> new file mode 100644
> index 000000000000..5158dbea8c43
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_const.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Isovalent */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +const volatile long foo = 42;
> +long bar;
> +long bart = 96;
> +
> +SEC("tc/ingress")
> +__description("rodata: write rejected")
> +__failure __msg("write into map forbidden")
> +int tcx1(struct __sk_buff *skb)
> +{
> +       char buff[] = { '8', '4', '\0' };
> +       bpf_strtol(buff, sizeof(buff), 0, (long *)&foo);
> +       return TCX_PASS;
> +}
> +
> +SEC("tc/ingress")
> +__description("bss: write accepted")
> +__success
> +int tcx2(struct __sk_buff *skb)
> +{
> +       char buff[] = { '8', '4', '\0' };
> +       bpf_strtol(buff, sizeof(buff), 0, &bar);
> +       return TCX_PASS;
> +}
> +
> +SEC("tc/ingress")
> +__description("data: write accepted")
> +__success
> +int tcx3(struct __sk_buff *skb)
> +{
> +       char buff[] = { '8', '4', '\0' };
> +       bpf_strtol(buff, sizeof(buff), 0, &bart);
> +       return TCX_PASS;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> --
> 2.43.0
>

Re: [PATCH bpf-next v4 1/8] bpf: Fix bpf_strtol and bpf_strtoul helpers for 32bit

[Andrii Nakryiko]

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> The bpf_strtol() and bpf_strtoul() helpers are currently broken on 32bit:
>
> The argument type ARG_PTR_TO_LONG is BPF-side "long", not kernel-side "long"
> and therefore always considered fixed 64bit no matter if 64 or 32bit underlying
> architecture.
>
> This contract breaks in case of the two mentioned helpers since their BPF_CALL
> definition for the helpers was added with {unsigned,}long *res. Meaning, the
> transition from BPF-side "long" (BPF program) to kernel-side "long" (BPF helper)
> breaks here.
>
> Both helpers call __bpf_strtoll() with "long long" correctly, but later assigning
> the result into 32-bit "*(long *)" on 32bit architectures. From a BPF program
> point of view, this means upper bits will be seen as uninitialised.
>
> Therefore, fix both BPF_CALL signatures to {s,u}64 types to fix this situation.
>
> Now, changing also uapi/bpf.h helper documentation which generates bpf_helper_defs.h
> for BPF programs is tricky: Changing signatures there to __{s,u}64 would trigger
> compiler warnings (incompatible pointer types passing 'long *' to parameter of type
> '__s64 *' (aka 'long long *')) for existing BPF programs.
>
> Leaving the signatures as-is would be fine as from BPF program point of view it is
> still BPF-side "long" and thus equivalent to __{s,u}64 on 64 or 32bit underlying
> architectures.
>
> Note that bpf_strtol() and bpf_strtoul() are the only helpers with this issue.
>

I think the bpf_helper_defs.h (and UAPI comment) side is completely
correct and valid. That "long" in BPF helpers is always s64 and we
rely on that on the BPF side. So I don't think we even need to change
anything there.

Perhaps the original intent was to do arch-native long, not sure...
But in any case, it's easy to check this in user code given we always
have full 64-bit result.

LGTM:

Acked-by: Andrii Nakryiko <andrii@kernel.org>



> Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Link: https://lore.kernel.org/bpf/481fcec8-c12c-9abb-8ecb-76c71c009959@iogearbox.net
> ---
>  v3 -> v4:
>  - added patch
>
>  kernel/bpf/helpers.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 3956be5d6440..0cf42be52890 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -518,7 +518,7 @@ static int __bpf_strtoll(const char *buf, size_t buf_len, u64 flags,
>  }
>
>  BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
> -          long *, res)
> +          s64 *, res)
>  {
>         long long _res;
>         int err;
> @@ -543,7 +543,7 @@ const struct bpf_func_proto bpf_strtol_proto = {
>  };
>
>  BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> -          unsigned long *, res)
> +          u64 *, res)
>  {
>         unsigned long long _res;
>         bool is_negative;
> --
> 2.43.0
>