Re: [PATCH bpf] bpf: invalidate unused part of bpf_prog_pack

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Thu, Apr 21, 2022 at 10:09 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Apr 21, 2022 at 12:27 AM Song Liu <song@kernel.org> wrote:
> >
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -228,6 +228,28 @@ static void jit_fill_hole(void *area, unsigned int size)
> >         memset(area, 0xcc, size);
> >  }
> >
> > +#define INVALID_BUF_SIZE PAGE_SIZE
> > +static char invalid_insn_buf[INVALID_BUF_SIZE];
> > +
> > +static int __init bpf_init_invalid_insn_buf(void)
> > +{
> > +       jit_fill_hole(invalid_insn_buf, INVALID_BUF_SIZE);
> > +       return 0;
> > +}
> > +pure_initcall(bpf_init_invalid_insn_buf);
> > +
> > +void bpf_arch_invalidate_text(void *dst, size_t len)
> > +{
> > +       size_t i = 0;
> > +
> > +       while (i < len) {
> > +               size_t s = min_t(size_t, len - i, INVALID_BUF_SIZE);
> > +
> > +               bpf_arch_text_copy(dst + i, invalid_insn_buf, s);
> > +               i += s;
> > +       }
> > +}
>
> Why do we need this new infrastructure?
>
> Why bpf_arch_invalidate_text()?
>
> Why not jit_fill_hole() unconditionally?
>
> It seems a bit pointless to have page buffer for containing this data,
> when we already have a (trivial) function to fill an area with invalid
> instructions.
>
> On x86, it's literally just "memset(0xcc)" (ie all 'int3' instructions).
>
> And on most RISC architectures, it would be some variation of
> "memset32(TRAP_INSN)".
>
> And all bpf targets should already have that nicely as that
> jit_fill_hole() function, no?
>
> The pack-allocator bpf code already *does* that, and is already passed
> that function.
>
> But it's just that it does it too late. Instead of doing it when
> allocating a new pack, it does it in the sub-allocator.
>
> Afaik the code in bpf/core.c already has all the information it needs,
> and already has that jit_fill_hole() function pointer, but is applying
> it at the wrong point.
>
> So I think the fix should be to just pass in that 'bpf_fill_ill_insns'
> function pointer all the way to alloc_new_pack(), instead of using it
> in bpf_jit_binary_alloc().

jit_fill_hole is an overkill here.

Long ago when jit spraying attack was fixed there was
a concern that memset(0) essentially populates the code page
with valid 'add BYTE PTR [rax],al' instructions.
Jumping anywhere in the zero page with a valid address in rax will
eventually lead to execution of the first insn in jit-ed bpf prog.
So memset(0xcc) was added to make it a bit harder to guess
the start address.
jit spraying is only a concern for archs that can
jump in the middle of the instruction and cpus will interpret
the byte stream differently.
The existing bpf_prog_pack code still does memset(0xcc)
a random range of bytes before and after jit-ed bpf code.
So doing memset(0xcc) for the whole huge page is not necessary at all.
Just memset(0) of a huge page at init time and memset(0)
when prog is freed is enough.
Jumping into zero page of 'valid' insns the cpu
will eventually stumble on 0xcc before reaching the first insn.
Let's not complicate the logic by dragging jit_fill_hole
further into generic allocation.