[RFC,3/5] bpf: fix validation of eal_divmod
Checks
Commit Message
Coverity spotted self assignment in BPF eval_divmod.
This looks like a bug where the incoming source register
should have been used instead.
Coverity issue: 302850
Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
lib/librte_bpf/bpf_validate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Tuesday, November 6, 2018 9:49 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
>
> Coverity spotted self assignment in BPF eval_divmod.
Yep, there is one.
As I remember I have to add it because one of old versions
of compiler (clang???) complained about 'variable being used uninitialized'.
> This looks like a bug where the incoming source register
> should have been used instead.
Nope, that's a wrong guess.
We shouldn't do it here.
Konstantin
>
> Coverity issue: 302850
> Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> lib/librte_bpf/bpf_validate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> index 83983efc4e5c..b768f72c4c02 100644
> --- a/lib/librte_bpf/bpf_validate.c
> +++ b/lib/librte_bpf/bpf_validate.c
> @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> if (op == BPF_MOD)
> rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> else
> - rd->u.max = rd->u.max;
> + rd->u.max = rs->u.max;
> rd->u.min = 0;
> }
>
> --
> 2.17.1
On Wed, 7 Nov 2018 12:54:54 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Tuesday, November 6, 2018 9:49 PM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> >
> > Coverity spotted self assignment in BPF eval_divmod.
>
> Yep, there is one.
> As I remember I have to add it because one of old versions
> of compiler (clang???) complained about 'variable being used uninitialized'.
>
> > This looks like a bug where the incoming source register
> > should have been used instead.
>
> Nope, that's a wrong guess.
> We shouldn't do it here.
> Konstantin
>
> >
> > Coverity issue: 302850
> > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > lib/librte_bpf/bpf_validate.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > index 83983efc4e5c..b768f72c4c02 100644
> > --- a/lib/librte_bpf/bpf_validate.c
> > +++ b/lib/librte_bpf/bpf_validate.c
> > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> > if (op == BPF_MOD)
> > rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> > else
> > - rd->u.max = rd->u.max;
> > + rd->u.max = rs->u.max;
> > rd->u.min = 0;
> > }
> >
> > --
> > 2.17.1
>
Well it was being used unintialized, your trick of self assignment fooled clang
but did not fool Coverity. What does the other BPF validator do?
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, November 7, 2018 7:52 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
>
> On Wed, 7 Nov 2018 12:54:54 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Tuesday, November 6, 2018 9:49 PM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> > >
> > > Coverity spotted self assignment in BPF eval_divmod.
> >
> > Yep, there is one.
> > As I remember I have to add it because one of old versions
> > of compiler (clang???) complained about 'variable being used uninitialized'.
> >
> > > This looks like a bug where the incoming source register
> > > should have been used instead.
> >
> > Nope, that's a wrong guess.
> > We shouldn't do it here.
> > Konstantin
> >
> > >
> > > Coverity issue: 302850
> > > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > > lib/librte_bpf/bpf_validate.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > > index 83983efc4e5c..b768f72c4c02 100644
> > > --- a/lib/librte_bpf/bpf_validate.c
> > > +++ b/lib/librte_bpf/bpf_validate.c
> > > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> > > if (op == BPF_MOD)
> > > rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> > > else
> > > - rd->u.max = rd->u.max;
> > > + rd->u.max = rs->u.max;
> > > rd->u.min = 0;
> > > }
> > >
> > > --
> > > 2.17.1
> >
>
> Well it was being used unintialized,
I don't think so, but if you can point to me where
exactly it is used uninitialized, we can discuss it further.
> your trick of self assignment fooled clang
It was one particular and pretty old version of clang
(if my memory serves me right).
With latest versions (let say 6.0) it doesn't complain,
if I remove that self-assignment.
gcc also doesn't see any problem here.
That makes me think it was a false-positive with old
version of the compiler.
Konstantin
> but did not fool Coverity. What does the other BPF validator do?
> > > >
> > > > Coverity spotted self assignment in BPF eval_divmod.
> > >
> > > Yep, there is one.
> > > As I remember I have to add it because one of old versions
> > > of compiler (clang???) complained about 'variable being used uninitialized'.
> > >
> > > > This looks like a bug where the incoming source register
> > > > should have been used instead.
> > >
> > > Nope, that's a wrong guess.
> > > We shouldn't do it here.
> > > Konstantin
> > >
> > > >
> > > > Coverity issue: 302850
> > > > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > ---
> > > > lib/librte_bpf/bpf_validate.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > > > index 83983efc4e5c..b768f72c4c02 100644
> > > > --- a/lib/librte_bpf/bpf_validate.c
> > > > +++ b/lib/librte_bpf/bpf_validate.c
> > > > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> > > > if (op == BPF_MOD)
> > > > rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> > > > else
> > > > - rd->u.max = rd->u.max;
> > > > + rd->u.max = rs->u.max;
> > > > rd->u.min = 0;
> > > > }
> > > >
> > > > --
> > > > 2.17.1
> > >
> >
> > Well it was being used unintialized,
>
> I don't think so, but if you can point to me where
> exactly it is used uninitialized, we can discuss it further.
>
> > your trick of self assignment fooled clang
>
> It was one particular and pretty old version of clang
> (if my memory serves me right).
> With latest versions (let say 6.0) it doesn't complain,
> if I remove that self-assignment.
> gcc also doesn't see any problem here.
> That makes me think it was a false-positive with old
> version of the compiler.
> Konstantin
As a another thought - it wouldn't take much effort to
send a patch with NOP self-assignment removed.
If it will pass our build-harness test, then it is probably
ok to integrate.
Konstantin
>
> > but did not fool Coverity. What does the other BPF validator do?
@@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
if (op == BPF_MOD)
rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
else
- rd->u.max = rd->u.max;
+ rd->u.max = rs->u.max;
rd->u.min = 0;
}