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 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 ; 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" 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: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> >> 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 ? 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