On Thu, Apr 21, 2022 at 12:27 AM Song Liu <[email protected]> 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().
NOTE! Once again, I'm not actually all that familiar with the code.
Maybe I'm missing something.
Linus
On Thu, Apr 21, 2022 at 10:09 AM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, Apr 21, 2022 at 12:27 AM Song Liu <[email protected]> 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.
On Thu, Apr 21, 2022 at 11:24 AM Alexei Starovoitov
<[email protected]> wrote:
>
> Let's not complicate the logic by dragging jit_fill_hole
> further into generic allocation.
I agree that just zeroing the page is probably perfectly fine in
practice on x86, but I'm also not really seeing the "complication" of
just doing things right.
> The existing bpf_prog_pack code still does memset(0xcc)
> a random range of bytes before and after jit-ed bpf code.
That is actually wishful thinking, and not based on reality.
From what I can tell, the end of the jit'ed bpf code is actually the
exception table entries, so we have that data being marked executable.
Honestly, what is wrong with this trivial patch?
I've not *tested* it, but it looks really really simple to me. Take it
as a "something like this" rather than anything else.
And yes, it would be better if bpf_jit_binary_pack_free did it too, so
that you don't have random old JIT'ed code lying around (and possibly
still in CPU branch history caches or whatever).
And it would be lovely if the exception table entries would be part of
another allocation and not marked executable.
But I certainly don't see the _downside_ (or complexity) of just doing
this, instead of zeroing things.
So this is by no means perfect, but it seems at least incrementally
_better_ than just zeroing. No?
Linus
Hi Linus,
On Thu, Apr 21, 2022 at 11:59 AM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, Apr 21, 2022 at 11:24 AM Alexei Starovoitov
> <[email protected]> wrote:
> >
> > Let's not complicate the logic by dragging jit_fill_hole
> > further into generic allocation.
>
> I agree that just zeroing the page is probably perfectly fine in
> practice on x86, but I'm also not really seeing the "complication" of
> just doing things right.
>
> > The existing bpf_prog_pack code still does memset(0xcc)
> > a random range of bytes before and after jit-ed bpf code.
>
> That is actually wishful thinking, and not based on reality.
>
> From what I can tell, the end of the jit'ed bpf code is actually the
> exception table entries, so we have that data being marked executable.
>
> Honestly, what is wrong with this trivial patch?
This version would fill the memory with illegal instruction when we
allocate the bpf_prog_pack.
The extra logic I had in the original patch was to erase the memory
when a BPF program is freed. In this case, the memory will be
returned to the bpf_prog_pack, and stays as RO+X. Actually, I
am not quite sure whether we need this logic. If not, we only need
the much simpler version.
Thanks,
Song
On Thu, Apr 21, 2022 at 12:41 PM Song Liu <[email protected]> wrote:
>
> The extra logic I had in the original patch was to erase the memory
> when a BPF program is freed. In this case, the memory will be
> returned to the bpf_prog_pack, and stays as RO+X. Actually, I
> am not quite sure whether we need this logic. If not, we only need
> the much simpler version.
Oh, I think it would be good to do at free time too.
I just would want that to use the same function we already have for
the allocation-time thing, instead of introducing completely new
infrastructure. That was what looked very odd to me.
Now, the _smallest_ patch would likely be to just save away that
'bpf_fill_ill_insns' function pointer in the 'struct bpf_prog_pack'
thing.
It's admittedly kind of silly to do, but it matches that whole silly
"let's pass around a function pointer to a fixed function" model at
allocation time.
I say that's silly, because it's a fixed architecture function and we
could just call it directly. The only valid function there is
jit_fill_hole(), and the only reason it uses that function pointer
seems to be that it's never been exposed as a real function.
So passing it along as a function seems to be _purely_ for the silly
reason that people haven't agreed on a name, and different
architectures use different names (ie power uses
'bpf_jit_fill_ill_insns()', RISC-V calls it 'bpf_fill_ill_insns()',
and everybody else seems to use 'jit_fill_hole'.
I don't know why that decision was made. It looks like a bad one to
me, honestly.
Why not just agree on a name - I suggest 'bpf_jit_fill_hole()' - and
just get rid of that stupid 'bpf_jit_fill_hole_t' type name that only
exists because of this thing?
The bpf headers seem to literally have agreed on a name for that
function -type- only in order to be able to disagree on the name of
the function -name-, and then pass it along as a function pointer
argument instead of just calling it directly.
Very counter-productive.
Linus