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

Commit Message

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

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 <[e-mail]>
Signed-off-by: Daniel Borkmann <[e-mail]>
Link: https://lore.kernel.org/bpf/[e-mail]
---
 v3 -> v4:
 - added patch

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

Comments

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

On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <[e-mail]> 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 <[e-mail]>



> Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
> Reported-by: Alexei Starovoitov <[e-mail]>
> Signed-off-by: Daniel Borkmann <[e-mail]>
> Link: https://lore.kernel.org/bpf/[e-mail]
> ---
>  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

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;