From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.warmcat.com (mail.warmcat.com [163.172.24.82]) by dpdk.org (Postfix) with ESMTP id AE5BF1BA0E for ; Tue, 15 May 2018 12:41:16 +0200 (CEST) To: Jerin Jacob Cc: dev@dpdk.org, thomas@monjalon.net, ferruh.yigit@intel.com, Pablo de Lara References: <20180515100335.13051-1-jerin.jacob@caviumnetworks.com> <54ecbab4-185e-2da6-8733-f40cc0648aa1@warmcat.com> <20180515103130.GA14446@jerin> From: Andy Green Message-ID: <007d01ed-1a2c-ce74-7ce5-b8e71b41b06d@warmcat.com> Date: Tue, 15 May 2018 18:41:03 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 In-Reply-To: <20180515103130.GA14446@jerin> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] bus/pci: fix driver name string manipulation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 May 2018 10:41:16 -0000 On 05/15/2018 06:31 PM, Jerin Jacob wrote: > -----Original Message----- >> Date: Tue, 15 May 2018 18:19:19 +0800 >> From: Andy Green >> To: Jerin Jacob , dev@dpdk.org >> CC: thomas@monjalon.net, ferruh.yigit@intel.com, Pablo de Lara >> >> Subject: Re: [dpdk-dev] [PATCH] bus/pci: fix driver name string manipulation >> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 >> Thunderbird/52.7.0 >> >> >> >> On 05/15/2018 06:03 PM, Jerin Jacob wrote: >>> sizeof(dri_name) is 8B on 64Bit systems.The intended operation is coping >>> the string after '/' from the string `name`. >>> >>> This bug is not letting to probe any device string >8B hence results in >>> the testpmd error("No ethernet devices found) on some PMDs. >> >> You are right... but... >> >>> Cc: Andy Green >>> Cc: Pablo de Lara >>> >>> Fixes: fe5f777b538 ("bus/pci: replace strncpy by strlcpy") >>> >>> Signed-off-by: Jerin Jacob >>> --- >>> drivers/bus/pci/linux/pci.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c >>> index a73ee49c2..cd45875b1 100644 >>> --- a/drivers/bus/pci/linux/pci.c >>> +++ b/drivers/bus/pci/linux/pci.c >>> @@ -54,7 +54,7 @@ pci_get_kernel_driver_by_path(const char *filename, char *dri_name) >>> name = strrchr(path, '/'); >>> if (name) { >>> - strlcpy(dri_name, name + 1, sizeof(dri_name)); >>> + strlcpy(dri_name, name + 1, strlen(name)); >> >> ... this fix is no good. The underlying problem is the length of dri_name >> is not getting passed into this function... it just doesn't know how much of >> dri_name is safe to use. Telling it to use the strlen() of something >> unrelated is going to make buffer overflows possible. > > In this case, already there is a check for following in the code. So it is fine > either way. > > path[count] = '\0'; > name = strrchr(path, '/'); I understand. But if any of that code changes, the copy can overrun. It's more robust if the copy limit is directly referenced to the destination size. >> I sent a patch to the list a few hours ago that amends this function to take >> the allocated length of dri_name, and sets the limit for the strlcpy() to >> that, so no matter what turns up in name it's not possible to blow past >> dri_name allocation. >> >> [dpdk-dev] [PATCH] bus/pci: correct the earlier strlcpy conversion​ > > I am fine with taking any of the of the patch. Please improve the patch subject > and bug description in case if you prefer to take your original patch. OK. Or the maintainer is welcome to change the commit title how he likes directly. -Andy >> >> -Andy >> >>> return 0; >>> } >>>