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 C2FEAA0C4D; Wed, 16 Jun 2021 19:28:04 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 838FC40683; Wed, 16 Jun 2021 19:28:04 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id 3FFDB4067A for ; Wed, 16 Jun 2021 19:28:02 +0200 (CEST) IronPort-SDR: ep5RA01GyBxeaq4P3zZPz7ipsDNRk4GXvi8pPPhKBknJsbzgX3Pt2DqnnrS28DvSpIBnElPb7Z DKyWDSygMU7w== X-IronPort-AV: E=McAfee;i="6200,9189,10016"; a="193533703" X-IronPort-AV: E=Sophos;i="5.83,278,1616482800"; d="scan'208";a="193533703" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Jun 2021 10:28:00 -0700 IronPort-SDR: V136Xn15VPbOOyogz3s9xS2JcQFvZizSmkPztmAON6XaWZhPfQo5fwNPHJg+apff5qRDz9CRT5 BRdLVPQdMKfQ== X-IronPort-AV: E=Sophos;i="5.83,278,1616482800"; d="scan'208";a="421570016" Received: from smcintyr-mobl1.ger.corp.intel.com (HELO [10.252.16.28]) ([10.252.16.28]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Jun 2021 10:27:59 -0700 To: ohilyard@iol.unh.edu Cc: dev@dpdk.org, david.marchand@redhat.com, Stephen Hemminger References: <20210616160730.348523-1-ohilyard@iol.unh.edu> <20210616095632.0f97eeb3@hermes.local> From: "Medvedkin, Vladimir" Message-ID: <983355ce-f2f7-ecc0-45be-9dad64ab979f@intel.com> Date: Wed, 16 Jun 2021 20:27:41 +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: <20210616095632.0f97eeb3@hermes.local> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] 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, Thanks for the fix. I like your solution with removing the loop. However, while this fixes the buffer overflow, IMO it is not complete, because get_dir() shouldn't be called in cases where depth = 128. In this case checking the MSB of the ip is not quite right thing. The only place where it is possible (depth == 128) is on calling get_nxt_node() from rte_rib6_lookup(), so I would suggest adding something like this: if (depth == 128) return NULL; to get_nxt_node() along with your changes. Also, apart from Stephen's comments, please add the corresponding fixline to the v2. Thanks! On 16/06/2021 19:56, Stephen Hemminger wrote: > On Wed, 16 Jun 2021 12:07:29 -0400 > 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. >> >> Signed-off-by: Owen Hilyard >> --- >> lib/rib/rte_rib6.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c >> index f6c55ee45..2de50449d 100644 >> --- a/lib/rib/rte_rib6.c >> +++ b/lib/rib/rte_rib6.c >> @@ -79,14 +79,20 @@ 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; >> + int index, msk; >> + /* depth & 127 clamps depth to values that will not > > Please put blank line after declarations for clarity. > Since index and mask are not signed values, please make them unsigned. > Better yet, make them sized to the appropriate number of bits. > >> + * 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; >> + /* >> + * 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 * > -- Regards, Vladimir