On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> Loading programs that use bpf_usdt_arg() on s390x fails with:
>
> ; switch (arg_spec->arg_type) {
> 139: (61) r1 = *(u32 *)(r2 +8)
> R2 unbounded memory access, make sure to bounds check any such access
can you show a bit longer log? we shouldn't just use
bpf_probe_read_kernel for this. I suspect strategically placed
barrier_var() calls will solve this. This is usually an issue with
compiler reordering operations and doing actual check after it already
speculatively adjusted pointer (which is technically safe and ok if we
never deref that pointer, but verifier doesn't recognize such pattern)
>
> The bound checks seem to be already in place in the C code, and maybe
> it's even possible to add extra bogus checks to placate LLVM or the
> verifier. Take a simpler approach and just use a helper.
>
> Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> ---
> tools/lib/bpf/usdt.bpf.h | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> index fdfd235e52c4..ddfa2521ab67 100644
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -116,7 +116,7 @@ __weak __hidden
> int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> {
> struct __bpf_usdt_spec *spec;
> - struct __bpf_usdt_arg_spec *arg_spec;
> + struct __bpf_usdt_arg_spec arg_spec;
> unsigned long val;
> int err, spec_id;
>
> @@ -133,21 +133,24 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
> return -ENOENT;
>
> - arg_spec = &spec->args[arg_num];
> - switch (arg_spec->arg_type) {
> + err = bpf_probe_read_kernel(&arg_spec, sizeof(arg_spec), &spec->args[arg_num]);
> + if (err)
> + return err;
> +
> + switch (arg_spec.arg_type) {
> case BPF_USDT_ARG_CONST:
> /* Arg is just a constant ("-4@$-9" in USDT arg spec).
> - * value is recorded in arg_spec->val_off directly.
> + * value is recorded in arg_spec.val_off directly.
> */
> - val = arg_spec->val_off;
> + val = arg_spec.val_off;
> break;
> case BPF_USDT_ARG_REG:
> /* Arg is in a register (e.g, "8@%rax" in USDT arg spec),
> * so we read the contents of that register directly from
> * struct pt_regs. To keep things simple user-space parts
> - * record offsetof(struct pt_regs, <regname>) in arg_spec->reg_off.
> + * record offsetof(struct pt_regs, <regname>) in arg_spec.reg_off.
> */
> - err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
> + err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec.reg_off);
> if (err)
> return err;
> break;
> @@ -155,18 +158,18 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> /* Arg is in memory addressed by register, plus some offset
> * (e.g., "-4@-1204(%rbp)" in USDT arg spec). Register is
> * identified like with BPF_USDT_ARG_REG case, and the offset
> - * is in arg_spec->val_off. We first fetch register contents
> + * is in arg_spec.val_off. We first fetch register contents
> * from pt_regs, then do another user-space probe read to
> * fetch argument value itself.
> */
> - err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
> + err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec.reg_off);
> if (err)
> return err;
> - err = bpf_probe_read_user(&val, sizeof(val), (void *)val + arg_spec->val_off);
> + err = bpf_probe_read_user(&val, sizeof(val), (void *)val + arg_spec.val_off);
> if (err)
> return err;
> #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> - val >>= arg_spec->arg_bitshift;
> + val >>= arg_spec.arg_bitshift;
> #endif
> break;
> default:
> @@ -177,11 +180,11 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> * necessary upper arg_bitshift bits, with sign extension if argument
> * is signed
> */
> - val <<= arg_spec->arg_bitshift;
> - if (arg_spec->arg_signed)
> - val = ((long)val) >> arg_spec->arg_bitshift;
> + val <<= arg_spec.arg_bitshift;
> + if (arg_spec.arg_signed)
> + val = ((long)val) >> arg_spec.arg_bitshift;
> else
> - val = val >> arg_spec->arg_bitshift;
> + val = val >> arg_spec.arg_bitshift;
> *res = val;
> return 0;
> }
> --
> 2.39.1
>
