From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1DF57A0613 for ; Tue, 30 Jul 2019 18:18:39 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EA0001BEFF; Tue, 30 Jul 2019 18:18:38 +0200 (CEST) Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id 98CB61BEF8 for ; Tue, 30 Jul 2019 18:18:34 +0200 (CEST) Received: by mail-wr1-f65.google.com with SMTP id n9so41353253wrr.4 for ; Tue, 30 Jul 2019 09:18:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=F4yurEGBUAR5zIpV1ULY12nLzNACr3hd//7vT3HMSR0=; b=b+a3UNC4bqgaXeXqBn8i6nJHl+2VJj6jZvc5Xf5fb0H/argseFVE/2rDmOE+Ti39zl qFOq2bRlotGqzTYAK2kiymOK2hmEvPCVUI+czc7DXpnXeZXOop9pSzlecANbnYSoRJqc mqTaW/7ROcy7yOaLStyoXaVwHAFAInZP8wM4CIVRuTg2naPI1JFf5g9OPhuCgiHWjggH lcNk5Sm0qyHxKsBCTH4vdRGvbSA+nHOyNLTr/z22LCoW9Ravqn/HNRmu+yIzzs0cSXKT AeK5JpWj1FqB1N17QEBHqXIZtiduFtMiNFQDZAJwhkFEEoMzGukWFzkfSR55Cd45EtR9 NG+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=F4yurEGBUAR5zIpV1ULY12nLzNACr3hd//7vT3HMSR0=; b=pUgMBisDA+mEDr0zVurB2MRpRQnPH/VnyxLCAMwFnr6XFjuhVI9m4nWSdIbaz78aZY 5i/vJbvT8IVu/rpE6KxJ6gQGe72K5RVKBj4kaTc8+GkojCTlq65sNs4bdWtnplqJP+e0 Lh3iwpLgV/W4aBsHQTU0BkNm6syhRRvGf3bXBQGpO53klZjWv1ESNTW18WGJHl6atuaI x0XcAtJo3qF4UefjLeKlNNbgLmDln657iosAPl1CcK3Kkth50m3AaBhZmvaV+/j1PJDa QvcDPGr+aYUjY3rGBjveeRVO9yvG/hreDufNDYj/Fv9QrMomyyaNbVXrKkbohYo0slGs jlBQ== X-Gm-Message-State: APjAAAWr1f8k7hgru0YeMnLKEeNAcuD8EMdqgFSA3Q62ZbSIadHwdffh Yn5vDWw1ywfS7pz6LKoXYBCj7g== X-Google-Smtp-Source: APXvYqwbf6uQSpbpLpBM7AD8QFZCCk3V15a3RA2HvsQkruu+374umv2PlLFM9M9P6LUo0QqBL+O3uw== X-Received: by 2002:adf:ec49:: with SMTP id w9mr120087730wrn.303.1564503514505; Tue, 30 Jul 2019 09:18:34 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id f2sm61879032wrq.48.2019.07.30.09.18.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 Jul 2019 09:18:33 -0700 (PDT) Date: Tue, 30 Jul 2019 18:18:32 +0200 From: Adrien Mazarguil To: Ferruh Yigit Cc: Aaron Conole , David Marchand , Bernard Iremonger , dev , dpdk stable , Thomas Monjalon , "Singh, Jasvinder" , Flavia Musatescu Message-ID: <20190730161831.GE4512@6wind.com> References: <1562670596-27129-1-git-send-email-bernard.iremonger@intel.com> <10372251.KTS5ePcUbj@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] librte_flow_classify: fix out-of-bounds access X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" 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 writes: > > > >> On Wed, Jul 10, 2019 at 11:49 PM Thomas Monjalon 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 > >>> > >>> 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