On Tue, Feb 16, 2021 at 11:33 AM Brendan Jackman <jackmanb@xxxxxxxxxx> wrote:
>
> On Mon, 15 Feb 2021 at 22:09, KP Singh <kpsingh@xxxxxxxxxx> wrote:
> >
> > On Mon, Feb 15, 2021 at 5:00 PM Brendan Jackman <jackmanb@xxxxxxxxxx> wrote:
> > >
> > > This code generates a CMPXCHG loop in order to implement atomic_fetch
> > > bitwise operations. Because CMPXCHG is hard-coded to use rax (which
> > > holds the BPF r0 value), it saves the _real_ r0 value into the
> > > internal "ax" temporary register and restores it once the loop is
> > > complete.
> > >
> > > In the middle of the loop, the actual bitwise operation is performed
> > > using src_reg. The bug occurs when src_reg is r0: as described above,
> > > r0 has been clobbered and the real r0 value is in the ax register.
> > >
> > > Therefore, perform this operation on the ax register instead, when
> > > src_reg is r0.
> > >
> > > Fixes: 981f94c3e921 ("bpf: Add bitwise atomic instructions")
> > > Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
> > > ---
> > > arch/x86/net/bpf_jit_comp.c | 7 +++---
> > > .../selftests/bpf/verifier/atomic_and.c | 23 +++++++++++++++++++
> > > 2 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 79e7a0ec1da5..0c9edfe42340 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -1349,6 +1349,7 @@ st: if (is_imm8(insn->off))
> > > insn->imm == (BPF_XOR | BPF_FETCH)) {
> > > u8 *branch_target;
> > > bool is64 = BPF_SIZE(insn->code) == BPF_DW;
> > > + u32 real_src_reg = src_reg == BPF_REG_0 ? BPF_REG_AX : src_reg;
> >
> > I think it would be more readable as:
> >
> > u32 real_src_reg = src_reg;
> >
> > /* Add a comment here why this is needed */
> > if (src_reg == BPF_REG_0)
> > real_src_reg = BPF_REG_AX;
>
> Yes good idea - actually if I put it next to the relevant mov:
>
> /* Will need RAX as a CMPXCHG operand so save R0 */
> emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0)
> if (src_reg == BPF_REG_0)
> real_src_reg = BPF_REG_AX;
>
> I don't think it even needs a comment - what do you think?
Yeah moving it there makes sense and you already have a comment there.
