From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3CFDAA0C42; Fri, 18 Jun 2021 18:22:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BE05740150; Fri, 18 Jun 2021 18:22:21 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id D27D140142 for ; Fri, 18 Jun 2021 18:22:19 +0200 (CEST) IronPort-SDR: l6TOndt2OhxFY33/VJosbWHKSHJp6boBdc0zw1LwnFdu+BhYIvSq+pC/AM01vQjwNqe8jdCisi wf1kNcya75xg== X-IronPort-AV: E=McAfee;i="6200,9189,10019"; a="292209023" X-IronPort-AV: E=Sophos;i="5.83,284,1616482800"; d="scan'208";a="292209023" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jun 2021 09:22:18 -0700 IronPort-SDR: DR+bn7CDO8U4oN/QVuWh2brPXsZfKiCL1Dx3Ml4HVqIVKzGp36x1i64dZZZr1l4Ur5V+7w0ko3 xQ3WyROziQYw== X-IronPort-AV: E=Sophos;i="5.83,284,1616482800"; d="scan'208";a="555622369" 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:22:17 -0700 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> From: "Medvedkin, Vladimir" Message-ID: <9f3f01d2-c476-88fc-c028-270b766fd215@intel.com> Date: Fri, 18 Jun 2021 19:22:14 +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: <20210616181833.356159-1-ohilyard@iol.unh.edu> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Owen, Just a few nits inlined below On 16/06/2021 21:18, ohilyard@iol.unh.edu wrote: > From: Owen Hilyard > > 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 > --- > 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 ? > + > + /* > + * 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