DPDK patches and discussions
 help / color / mirror / Atom feed
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: Aaron Conole <aconole@redhat.com>,
	David Marchand <david.marchand@redhat.com>,
	Bernard Iremonger <bernard.iremonger@intel.com>,
	dev <dev@dpdk.org>, dpdk stable <stable@dpdk.org>,
	Thomas Monjalon <thomas@monjalon.net>,
	"Singh, Jasvinder" <jasvinder.singh@intel.com>,
	Flavia Musatescu <flavia.musatescu@intel.com>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] librte_flow_classify: fix out-of-bounds access
Date: Tue, 30 Jul 2019 18:18:32 +0200	[thread overview]
Message-ID: <20190730161831.GE4512@6wind.com> (raw)
In-Reply-To: <c87285dd-f322-809f-7cc3-3b41593da1be@intel.com>

On Tue, Jul 30, 2019 at 03:48:31PM +0100, Ferruh Yigit wrote:
> On 7/30/2019 3:42 PM, Aaron Conole wrote:
> > David Marchand <david.marchand@redhat.com> writes:
> > 
> >> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>>
> >>> 09/07/2019 13:09, Bernard Iremonger:
> >>>> This patch fixes the out-of-bounds coverity issue by removing the
> >>>> offending line of code at line 107 in rte_flow_classify_parse.c
> >>>> which is never executed.
> >>>>
> >>>> Coverity issue: 343454
> >>>>
> >>>> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
> >>>> Cc: stable@dpdk.org
> >>>> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> >>>
> >>> Applied, thanks
> >>
> >> We have a segfault in the unit tests since this patch.
> > 
> > I think this patch is still correct.  The issue is in the semantic of
> > the flow classify pattern.  It *MUST* always have a valid end marker,
> > but the test passes an invalid end marker.  This causes the bounds to
> > exceed.
> > 
> > So, it would be best to fix it, either by having a "failure" on unknown
> > markers (f.e. -1), or by passing a length around.  However, the crash
> > should be expected.  The fact that the previous code was also incorrect
> > and resulted in no segfault is pure luck.
> > 
> > See rte_flow_classify_parse.c:80 and test_flow_classify.c:387
> > 
> > I would be in favor of passing the lengths of the two arrays to these
> > APIs.  That would let us still make use of the markers (for valid
> > construction), but also let us reason about lengths in a sane way.
> > 
> > WDYT?
> > 
> 
> +1, I also just replied with something very similar.
> 
> With current API the testcase is wrong, and it will crash, also the invalid
> action one has exact same problem.
> 
> The API can be updated as you suggested, with a length field and testcases can
> be added back.
> 
> What worries me more is the rte_flow, which uses same arguments, and open to
> same errors, should we consider updating rte_flow APIs to have lengths values too?

(Jumping in since all dashboard lights in my control room went red after
"rte_flow" was detected in this discussion)

Length values for patterns and action lists were considered during design
but END was preferred as the better solution for convenience and because
it's actually safer:

- C programmers are well aware of the dire consequences of omitting the
  ending NUL byte in strings so it's not a foreign concept. This is
  documented as such for rte_flow.

- Static initialization of flow rules (i.e. defining a large fixed array)
  is much easier if one doesn't have to encode its size as well, think about
  compilation directives (#ifdef) on some of its elements.

- Like omitting the END element, providing the wrong array size by mistake
  remains a possibility, with similar or possibly worse consequences as
  it's less likely to crash early and more prone to silent data corruption.

- [tons of other good reasons here]

See?

-- 
Adrien Mazarguil
6WIND

  parent reply	other threads:[~2019-07-30 16:18 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-09 11:09 [dpdk-dev] " Bernard Iremonger
2019-07-10 21:48 ` Thomas Monjalon
2019-07-29 13:09   ` David Marchand
2019-07-30 14:42     ` Aaron Conole
2019-07-30 14:48       ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2019-07-30 15:43         ` Aaron Conole
2019-07-30 16:55           ` Ferruh Yigit
2019-07-30 17:30             ` Aaron Conole
2019-07-30 16:18         ` Adrien Mazarguil [this message]
2019-07-30 16:35           ` Ferruh Yigit
2019-07-30 17:27             ` Aaron Conole
2019-07-30 18:51               ` Adrien Mazarguil
2019-07-30 14:44     ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190730161831.GE4512@6wind.com \
    --to=adrien.mazarguil@6wind.com \
    --cc=aconole@redhat.com \
    --cc=bernard.iremonger@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=flavia.musatescu@intel.com \
    --cc=jasvinder.singh@intel.com \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).