From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751740AbdCMM7R (ORCPT ); Mon, 13 Mar 2017 08:59:17 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:36079 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750858AbdCMM7J (ORCPT ); Mon, 13 Mar 2017 08:59:09 -0400 Date: Mon, 13 Mar 2017 15:57:46 +0300 From: Dan Carpenter To: SIMRAN SINGHAL Cc: Greg KH , devel@driverdev.osuosl.org, outreachy-kernel , arve@android.com, riandrews@android.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] staging: android: Replace strcpy with strlcpy Message-ID: <20170313125746.GD4187@mwanda> References: <20170311204001.GA13301@singhal-Inspiron-5558> <20170313124144.GE4136@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 13, 2017 at 06:17:22PM +0530, SIMRAN SINGHAL wrote: > On Mon, Mar 13, 2017 at 6:11 PM, Dan Carpenter wrote: > > On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote: > >> Replace strcpy with strlcpy as strcpy does not check for buffer > >> overflow. > >> This is found using Flawfinder. > >> > >> Signed-off-by: simran singhal > >> --- > >> drivers/staging/android/ashmem.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c > >> index 7cbad0d..eb2f4ef 100644 > >> --- a/drivers/staging/android/ashmem.c > >> +++ b/drivers/staging/android/ashmem.c > >> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name) > >> if (unlikely(asma->file)) > >> ret = -EINVAL; > >> else > >> - strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name); > >> + strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name, > >> + sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN)); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > This isn't right. > > > > Also please do some analysis to see if it's a real bug or a false > > positive. It is a false positive in this case. > > > > Dan, > I have already sent v3 of this in which I have used: > sizeof(asma->name) - ASHMEM_NAME_PREFIX_LEN Yeah. I saw that. It's fine, I suppose but you should have done more analysis to see if it was a real bug like Al and Greg suggested. The changelog should say something like: "The destination buffer is 12345 bytes long but we're copying a 10000 character string so it can overflow." Occasionally, I will fudge a little bit on these changelogs to say that I have looked every where to determine the size of the source buffer and can't figure it out so this change makes it easier to audit. But I try to figure it out generally. Really tools should be able to show that this code is safe. They currently don't so far as I know, but they should. It's a matter of waiting a year for Smatch to improve. regards, dan carpenter