From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B63E0A0C42; Fri, 18 Jun 2021 18:27:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 461A040150; Fri, 18 Jun 2021 18:27:44 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id 1253640142 for <dev@dpdk.org>; Fri, 18 Jun 2021 18:27:41 +0200 (CEST) IronPort-SDR: MNj4UHkezeofuJD6sAZp7oXFALHXM0UqqNfVyHj/LKtkS9p8NhmkqW1V7a8WZb9Qi1n5zV88OT jyrixngTzizw== X-IronPort-AV: E=McAfee;i="6200,9189,10019"; a="270428882" X-IronPort-AV: E=Sophos;i="5.83,284,1616482800"; d="scan'208";a="270428882" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jun 2021 09:27:40 -0700 IronPort-SDR: 5AQYENKugQuYD/bEM8jnfKdObLAfzNDAMibmJRyzcn+7kj5i4e4XOdCRRda/6pBV1b5aV/Cqyv hEIch+dUUwVg== X-IronPort-AV: E=Sophos;i="5.83,284,1616482800"; d="scan'208";a="555623506" Received: from vmedvedk-mobl.ger.corp.intel.com (HELO [10.252.30.101]) ([10.252.30.101]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jun 2021 09:27:37 -0700 From: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com> To: ohilyard@iol.unh.edu Cc: dev@dpdk.org, stephen@networkplumber.org, david.marchand@redhat.com References: <20210616160730.348523-1-ohilyard@iol.unh.edu> <20210616181833.356159-1-ohilyard@iol.unh.edu> <9f3f01d2-c476-88fc-c028-270b766fd215@intel.com> Message-ID: <c18d7072-21ed-1cec-5f40-61cc81c84f90@intel.com> Date: Fri, 18 Jun 2021 19:27:26 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <9f3f01d2-c476-88fc-c028-270b766fd215@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] lib/rte_rib6: fix stack buffer overflow X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> On 18/06/2021 19:22, Medvedkin, Vladimir wrote: > Hi Owen, > > Just a few nits inlined below > > On 16/06/2021 21:18, ohilyard@iol.unh.edu wrote: >> From: Owen Hilyard <ohilyard@iol.unh.edu> >> >> ASAN found a stack buffer overflow in lib/rib/rte_rib6.c:get_dir. >> The fix for the stack buffer overflow was to make sure depth >> was always < 128, since when depth = 128 it caused the index >> into the ip address to be 16, which read off the end of the array. >> >> While trying to solve the buffer overflow, I noticed that a few >> changes could be made to remove the for loop entirely. >> >> Fixes: f7e861e21c ("rib: support IPv6") >> >> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu> >> --- >> lib/rib/rte_rib6.c | 27 +++++++++++++++++++-------- >> 1 file changed, 19 insertions(+), 8 deletions(-) >> >> diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c >> index f6c55ee45..a4daf12ca 100644 >> --- a/lib/rib/rte_rib6.c >> +++ b/lib/rib/rte_rib6.c >> @@ -79,20 +79,31 @@ is_covered(const uint8_t >> ip1[RTE_RIB6_IPV6_ADDR_SIZE], >> static inline int >> get_dir(const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE], uint8_t depth) >> { >> - int i = 0; >> - uint8_t p_depth, msk; >> - >> - for (p_depth = depth; p_depth >= 8; p_depth -= 8) >> - i++; >> - >> - msk = 1 << (7 - p_depth); >> - return (ip[i] & msk) != 0; >> + uint8_t index, msk; >> + >> + /* depth & 127 clamps depth to values that will not > > Please be consistent with the coding style. Check 1.3.1 in > https://doc.dpdk.org/guides/contributing/coding_style.html > >> + * read off the end of ip. >> + * depth is the number of bits deep into ip to traverse, and >> + * is incremented in blocks of 8 (1 byte). This means the last >> + * 3 bits are irrelevant to what the index of ip should be. >> + */ >> + index = (depth & 127) >> 3; > > (depth & INT8_MAX) / UINT8_MAX ? Ugh... sed 's/UINT8_MAX/CHAR_BIT' > >> + >> + /* >> + * msk is the bitmask used to extract the bit used to decide the >> + * direction of the next step of the binary search. >> + */ >> + msk = 1 << (7 - (depth & 7)); >> + >> + return (ip[index] & msk) != 0; >> } >> static inline struct rte_rib6_node * >> get_nxt_node(struct rte_rib6_node *node, >> const uint8_t ip[RTE_RIB6_IPV6_ADDR_SIZE]) >> { >> + if (node->depth == 128) >> + return NULL; > > please use RIB6_MAXDEPTH instead of 128. > Also I'd put a blank line before the final return. > >> return (get_dir(ip, node->depth)) ? node->right : node->left; >> } >> > > while this is a bug fix, please add Cc: stable@dpdk.org on v3 to > backport this patch. > > Apart from that LGTM. > -- Regards, Vladimir