linux-kernel - Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen parameter using &= sizeof(saddr)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fVU07VHcQE6r9k7aEV+xM3_HFcgY+5Y8N7qVvsZD3V9vg@mail.gmail.com>
Date:   Wed, 16 Aug 2023 15:10:00 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] perf bpf_skel augmented_raw_syscalls: Cap the socklen
 parameter using &= sizeof(saddr)

On Wed, Aug 16, 2023 at 2:48 PM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> This works with:
>
>   $ clang -v
>   clang version 14.0.5 (Fedora 14.0.5-2.fc36)
>   $
>
> But not with:
>
>   $ clang -v
>   clang version 16.0.6 (Fedora 16.0.6-2.fc38)
>   $
>
>   [root@...co ~]# perf trace -e connect*,sendto* ping -c 10 localhost
>   libbpf: prog 'sys_enter_sendto': BPF program load failed: Permission denied
>   libbpf: prog 'sys_enter_sendto': -- BEGIN PROG LOAD LOG --
>   reg type unsupported for arg#0 function sys_enter_sendto#59
>   0: R1=ctx(off=0,imm=0) R10=fp0
>   ; int sys_enter_sendto(struct syscall_enter_args *args)
>   0: (bf) r6 = r1                       ; R1=ctx(off=0,imm=0) R6_w=ctx(off=0,imm=0)
>   1: (b7) r1 = 0                        ; R1_w=0
>   ; int key = 0;
>   2: (63) *(u32 *)(r10 -4) = r1         ; R1_w=0 R10=fp0 fp-8=0000????
>   3: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
>   ;
>   4: (07) r2 += -4                      ; R2_w=fp-4
>   ; return bpf_map_lookup_elem(&augmented_args_tmp, &key);
>   5: (18) r1 = 0xffff8de5a5b8bc00       ; R1_w=map_ptr(off=0,ks=4,vs=8272,imm=0)
>   7: (85) call bpf_map_lookup_elem#1    ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0)
>   8: (bf) r7 = r0                       ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0) R7_w=map_value_or_null(id=1,off=0,ks=4,vs=8272,imm=0)
>   9: (b7) r0 = 1                        ; R0_w=1
>   ; if (augmented_args == NULL)
>   10: (15) if r7 == 0x0 goto pc+25      ; R7_w=map_value(off=0,ks=4,vs=8272,imm=0)
>   ; unsigned int socklen = args->args[5];
>   11: (79) r1 = *(u64 *)(r6 +56)        ; R1_w=scalar() R6_w=ctx(off=0,imm=0)
>   ;
>   12: (bf) r2 = r1                      ; R1_w=scalar(id=2) R2_w=scalar(id=2)
>   13: (67) r2 <<= 32                    ; R2_w=scalar(smax=9223372032559808512,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32_max=0)
>   14: (77) r2 >>= 32                    ; R2_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
>   15: (b7) r8 = 128                     ; R8=128
>   ; if (socklen > sizeof(augmented_args->saddr))
>   16: (25) if r2 > 0x80 goto pc+1       ; R2=scalar(umax=128,var_off=(0x0; 0xff))
>   17: (bf) r8 = r1                      ; R1=scalar(id=2) R8_w=scalar(id=2)
>   ; const void *sockaddr_arg = (const void *)args->args[4];
>   18: (79) r3 = *(u64 *)(r6 +48)        ; R3_w=scalar() R6=ctx(off=0,imm=0)
>   ; bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
>   19: (bf) r1 = r7                      ; R1_w=map_value(off=0,ks=4,vs=8272,imm=0) R7=map_value(off=0,ks=4,vs=8272,imm=0)
>   20: (07) r1 += 64                     ; R1_w=map_value(off=64,ks=4,vs=8272,imm=0)
>   ; bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
>   21: (bf) r2 = r8                      ; R2_w=scalar(id=2) R8_w=scalar(id=2)
>   22: (85) call bpf_probe_read#4
>   R2 min value is negative, either use unsigned or 'var &= const'
>   processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
>   -- END PROG LOAD LOG --
>   libbpf: prog 'sys_enter_sendto': failed to load: -13
>   libbpf: failed to load object 'augmented_raw_syscalls_bpf'
>   libbpf: failed to load BPF skeleton 'augmented_raw_syscalls_bpf': -13
>
> So use the suggested &= variant since sizeof(saddr) == 128 bytes.

Could this be an assert?

Thanks,
Ian

>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Cc: Ian Rogers <irogers@...gle.com>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> ---
>  tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> index 0586c4118656d3e4..9c1d0b271b20f693 100644
> --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> @@ -187,8 +187,7 @@ int sys_enter_connect(struct syscall_enter_args *args)
>          if (augmented_args == NULL)
>                  return 1; /* Failure: don't filter */
>
> -       if (socklen > sizeof(augmented_args->saddr))
> -               socklen = sizeof(augmented_args->saddr);
> +       socklen &= sizeof(augmented_args->saddr) - 1;
>
>         bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
>
> @@ -206,8 +205,7 @@ int sys_enter_sendto(struct syscall_enter_args *args)
>          if (augmented_args == NULL)
>                  return 1; /* Failure: don't filter */
>
> -       if (socklen > sizeof(augmented_args->saddr))
> -               socklen = sizeof(augmented_args->saddr);
> +       socklen &= sizeof(augmented_args->saddr) - 1;
>
>         bpf_probe_read(&augmented_args->saddr, socklen, sockaddr_arg);
>
> --
> 2.41.0
>