netdev - Re: [PATCH v3 1/3] bpf,x64: pad NOPs to make images converge more easily

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: <CAADnVQJiK3BWLr5LRhThUySC=6VyiP=tt3ttiyZPHGLmoU4jDg@mail.gmail.com>
Date:   Thu, 14 Jan 2021 22:37:33 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Gary Lin <glin@...e.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        andreas.taschner@...e.com
Subject: Re: [PATCH v3 1/3] bpf,x64: pad NOPs to make images converge more easily

On Thu, Jan 14, 2021 at 1:54 AM Gary Lin <glin@...e.com> wrote:
>          * pass to emit the final image.
>          */
> -       for (pass = 0; pass < 20 || image; pass++) {
> -               proglen = do_jit(prog, addrs, image, oldproglen, &ctx);
> +       for (pass = 0; pass < MAX_PASSES || image; pass++) {
> +               if (!padding && pass >= PADDING_PASSES)
> +                       padding = true;
> +               proglen = do_jit(prog, addrs, image, oldproglen, &ctx, padding);

I'm struggling to reconcile the discussion we had before holidays with
the discussion you guys had in v2:

>> What is the rationale for the latter when JIT is called again for subprog to fill in relative
>> call locations?
>>
> Hmmmm, my thinking was that we only enable padding for those programs
> which are already padded before. But, you're right. For the programs
> converging without padding, enabling padding won't change the final
> image, so it's safe to always set "padding" to true for the extra pass.
>
> Will remove the "padded" flag in v3.

I'm not following why "enabling padding won't change the final image"
is correct.
Say the subprog image converges without padding.
Then for subprog we call JIT again.
Now extra_pass==true and padding==true.
The JITed image will be different.
The test in patch 3 should have caught it, but it didn't,
because it checks for a subprog that needed padding.
The extra_pass needs to emit insns exactly in the right spots.
Otherwise jump targets will be incorrect.
The saved addrs[] array is crucial.
If extra_pass emits different things the instruction starts won't align
to places where addrs[] expects them to be.

So I think the padded flag has to be part of x64_jit_data.
Please double check my analysis and see why your test keeps working.
And please add another test that crashes with this v3 and works when
'padding' is saved.
I expected at least some tests in test_progs to be crashing, but
I've applied patch 1 and run the tests manually and everything passed,
so I could be missing something or our test coverage for subprogs is too weak.