[6/6] ebpf: allow BPF_REG_X in src_reg conditional jumps

On Fri, Sep 04, 2015 at 10:04:24AM -0600, Tycho Andersen wrote:
> The classic converter generates conditional jumps with:
> 
> if (BPF_SRC(fp->code) == BPF_K && (int) fp->k < 0) {
>         ...
> } else {
>         insn->dst_reg = BPF_REG_A;
>         insn->src_reg = BPF_REG_X;
>         insn->imm = fp->k;
>         bpf_src = BPF_SRC(fp->code);
> }
> 
> but here, we enforce that the src_reg == BPF_REG_0. We should also allow
> BPF_REG_X since that's what the converter generates; this enables us to
> load eBPF programs that were generated by the converter.

good catch. classic->extended converter is just being untidy.
It shouldn't be populating unused 'src_reg' field when BPF_SRC == BPF_K
verifier is doing the right thing. It's rejecting instructions that
have junk in unused fields to make sure that someday we can extend it
with something useful.
The fix should be something like this:
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Alexei,

On Fri, Sep 04, 2015 at 02:06:19PM -0700, Alexei Starovoitov wrote:
> On Fri, Sep 04, 2015 at 10:04:24AM -0600, Tycho Andersen wrote:
> > The classic converter generates conditional jumps with:
> > 
> > if (BPF_SRC(fp->code) == BPF_K && (int) fp->k < 0) {
> >         ...
> > } else {
> >         insn->dst_reg = BPF_REG_A;
> >         insn->src_reg = BPF_REG_X;
> >         insn->imm = fp->k;
> >         bpf_src = BPF_SRC(fp->code);
> > }
> > 
> > but here, we enforce that the src_reg == BPF_REG_0. We should also allow
> > BPF_REG_X since that's what the converter generates; this enables us to
> > load eBPF programs that were generated by the converter.
> 
> good catch. classic->extended converter is just being untidy.
> It shouldn't be populating unused 'src_reg' field when BPF_SRC == BPF_K
> verifier is doing the right thing. It's rejecting instructions that
> have junk in unused fields to make sure that someday we can extend it
> with something useful.
> The fix should be something like this:
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 13079f03902e..05a04ea87172 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -478,9 +478,9 @@ do_pass:
>                                 bpf_src = BPF_X;
>                         } else {
>                                 insn->dst_reg = BPF_REG_A;
> -                               insn->src_reg = BPF_REG_X;
>                                 insn->imm = fp->k;
>                                 bpf_src = BPF_SRC(fp->code);
> +                               insn->src_reg = bpf_src == BPF_X ? BPF_REG_X : 0;
>                         }

Yep, I just tested this and it works for me. Do you want to manage it
or should I carry it as part of this set?

Tycho
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 04, 2015 at 04:43:50PM -0600, Tycho Andersen wrote:
> > The fix should be something like this:
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 13079f03902e..05a04ea87172 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -478,9 +478,9 @@ do_pass:
> >                                 bpf_src = BPF_X;
> >                         } else {
> >                                 insn->dst_reg = BPF_REG_A;
> > -                               insn->src_reg = BPF_REG_X;
> >                                 insn->imm = fp->k;
> >                                 bpf_src = BPF_SRC(fp->code);
> > +                               insn->src_reg = bpf_src == BPF_X ? BPF_REG_X : 0;
> >                         }
> 
> Yep, I just tested this and it works for me. Do you want to manage it
> or should I carry it as part of this set?

Though it's a bug, it doesn't affect anything at the moment
and not worth fixing in net, so please submit it as separate bug
fix when net-next reopens. imo the rest of the patches should
also go via net-next to minimize cross-tree conflicts.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html