diff mbox series

[RFC,3/5] bpf: fix validation of eal_divmod

Message ID 20181106214901.1392-4-stephen@networkplumber.org (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series more Coverity related bug fixes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger Nov. 6, 2018, 9:48 p.m. UTC 
  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

Ananyev, Konstantin Nov. 7, 2018, 12:54 p.m. UTC | #1 
> -----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
Stephen Hemminger Nov. 7, 2018, 7:51 p.m. UTC | #2 
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?
Ananyev, Konstantin Nov. 7, 2018, 8:07 p.m. UTC | #3 
> -----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?
Ananyev, Konstantin Nov. 7, 2018, 11:04 p.m. UTC | #4 
> > > >
> > > > 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?
diff mbox series

Patch

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;
 	}