On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> Fixes: d7a4cb9b6705 ("bpf: Introduce bpf_strtol and bpf_strtoul helpers")
> Reported-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> Link: https://lore.kernel.org/bpf/481fcec8-c12c-9abb-8ecb-76c71c009959@xxxxxxxxxxxxx
> ---
> 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
>
