On Thu, Sep 3, 2020 at 6:10 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
> > On code patching it may require to update branch destinations if the
> > code size changed. bpf_adj_delta_to_imm/off increments offset only
> > if the patched area is after the branch instruction. But it's
> > possible, that the patched area itself is a branch instruction and
> > requires destination update.
>
> Could you provide a concrete example and walk us through? I'm probably
> missing something but if the patchlet contains a branch instruction, then
> it should be 'self-contained'. In the sense that the patchlet is a 'black
> box' that replaces 1 insns with n insns but there is no awareness what's
> inside these insns and hence no fixup for that inside bpf_patch_insn_data().
The code is
Disassembly of section classifier/test:
0000000000000000 test_cls:
0: 85 01 00 00 ff ff ff ff call -1
0000000000000000: R_BPF_64_32 f7
1: 95 00 00 00 00 00 00 00 exit
0000000000000000 f1:
0: 61 01 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
1: 95 00 00 00 00 00 00 00 exit
[...]
00000000000000a8 f7:
21: 85 01 00 00 ff ff ff ff call -1
00000000000000a8: R_BPF_64_32 f6
22: 95 00 00 00 00 00 00 00 exit
Before the patching the bytecode is:
00000000: 85 01 00 00 00 00 00 16 95 00 00 00 00 00 00 00
00000010: 61 01 00 00 00 00 00 00 95 00 00 00 00 00 00 00
[...]
It becomes
00000000: 85 01 00 00 00 00 00 2b bc 00 00 00 00 00 00 01
00000010: 95 00 00 00 00 00 00 00 61 01 00 80 00 00 00 00
at the end, the 2b offset is incorrect.
With that zext patching the code "85 01 00 00 00 00 00 16" is replaced
with "85 01 00 00 00 00 00 16 bc 00 00 00 00 00 00 01", 0x16 is not
changed, but the real offset has changed.
> So, if we take an existing branch insns from the code, move it into the
> patchlet and extend beginning or end, then it feels more like a bug to the
> one that called bpf_patch_insn_data(), aka zext code here. Bit puzzled why
> this is only seen now, my impression was that Ilya was running s390x the
> BPF selftests quite recently?
I have not investigated why on s390 it is zext'ed, but on x86 not,
it's related to the size of the register when it returns 32bit value.
There may be a bug there as well.
I did think a bit more on your words, making the zext patching code
specially check jumps and adjust the offset in the patchlet looks more
correct. But duplicates the existing code. I should spend more time on
that.
>
> > The problem was triggered by bpf selftest
> >
> > test_progs -t global_funcs
> >
> > on s390, where the very first "call" instruction is patched from
> > verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.
> >
> > The patch includes current instruction to the condition check.
> >
> > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@xxxxxxxxxx>
> > ---
> > kernel/bpf/core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index ed0b3578867c..b0a9a22491a5 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, s32 end_old,
> > s32 delta = end_new - end_old;
> > s64 imm = insn->imm;
> >
> > - if (curr < pos && curr + imm + 1 >= end_old)
> > + if (curr <= pos && curr + imm + 1 >= end_old)
> > imm += delta;
> > else if (curr >= end_new && curr + imm + 1 < end_new)
> > imm -= delta;
> > @@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, s32 end_old,
> > s32 delta = end_new - end_old;
> > s32 off = insn->off;
> >
> > - if (curr < pos && curr + off + 1 >= end_old)
> > + if (curr <= pos && curr + off + 1 >= end_old)
> > off += delta;
> > else if (curr >= end_new && curr + off + 1 < end_new)
> > off -= delta;
> >
>
--
WBR, Yauheni
