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: <CAEf4BzaF+pLa0vGFaQCfLrF=9pjqYmkUhSC7d8yS2-Dv3DCsMA@mail.gmail.com>
Date:   Tue, 6 Oct 2020 11:17:58 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Luka Perkov <luka.perkov@...tura.hr>,
        Tony Ambardar <tony.ambardar@...il.com>
Subject: Re: [PATCH bpf-next 1/3] libbpf: support safe subset of load/store
 instruction resizing with CO-RE

On Tue, Oct 6, 2020 at 11:08 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Thu, Oct 01, 2020 at 06:06:31PM -0700, Andrii Nakryiko wrote:
> > Add support for patching instructions of the following form:
> >   - rX = *(T *)(rY + <off>);
> >   - *(T *)(rX + <off>) = rY;
> >   - *(T *)(rX + <off>) = <imm>, where T is one of {u8, u16, u32, u64}.
>
> llvm doesn't generate ST instruction. It never did.
> STX is generated, but can it actually be used with relocations?

interesting, so whe you do `some_struct->some_field = 123;` Clang will still do:

r1 = 123;
*(u32 *)(r2 + 0) = r1;

?

I'll add a test with constant and see what gets generated.

To answer the second part, unless someone is willing to manually
generate .BTF.ext for assembly instruction, then no, you can't really
use it with CO-RE relocation.

> Looking at the test in patch 3... it's testing LDX only.
> ST/STX suppose to work by analogy, but would be good to have a test.
> At least of STX.

yep, will add.

>
> > +static int insn_mem_sz_to_bytes(struct bpf_insn *insn)
> > +{
> > +     switch (BPF_SIZE(insn->code)) {
> > +     case BPF_DW: return 8;
> > +     case BPF_W: return 4;
> > +     case BPF_H: return 2;
> > +     case BPF_B: return 1;
> > +     default: return -1;
> > +     }
> > +}
> > +
> > +static int insn_bytes_to_mem_sz(__u32 sz)
> > +{
> > +     switch (sz) {
> > +     case 8: return BPF_DW;
> > +     case 4: return BPF_W;
> > +     case 2: return BPF_H;
> > +     case 1: return BPF_B;
> > +     default: return -1;
> > +     }
> > +}
>
> filter.h has these two helpers. They're named bytes_to_bpf_size() and bpf_size_to_bytes().
> I guess we cannot really share kernel and libbpf implementation, but
> could you please name them the same way so it's easier to follow
> for folks who read both kernel and libbpf code?

yes, of course. I actually spent some time searching for them, but
couldn't find them in kernel code. I remembered seeing them
previously, but it never occurred to me to chekc filter.h.

>
> > +             if (res->new_sz != res->orig_sz) {
> > +                     mem_sz = insn_mem_sz_to_bytes(insn);
> > +                     if (mem_sz != res->orig_sz) {
> > +                             pr_warn("prog '%s': relo #%d: insn #%d (LDX/ST/STX) unexpected mem size: got %u, exp %u\n",
> > +                                     prog->name, relo_idx, insn_idx, mem_sz, res->orig_sz);
> > +                             return -EINVAL;
> > +                     }
> > +
> > +                     mem_sz = insn_bytes_to_mem_sz(res->new_sz);
> > +                     if (mem_sz < 0) {
>
> Please use new variable here appropriately named.
> Few lines above mem_sz is in bytes while here it's encoding opcode.

ok

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ