From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932781AbdCKVe4 (ORCPT ); Sat, 11 Mar 2017 16:34:56 -0500 Received: from mail2-relais-roc.national.inria.fr ([192.134.164.83]:13958 "EHLO mail2-relais-roc.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755340AbdCKVev (ORCPT ); Sat, 11 Mar 2017 16:34:51 -0500 X-IronPort-AV: E=Sophos;i="5.36,148,1486422000"; d="scan'208";a="264155607" Date: Sat, 11 Mar 2017 22:34:48 +0100 (CET) From: Julia Lawall X-X-Sender: jll@hadrien To: SIMRAN SINGHAL cc: Greg KH , arve@android.com, riandrews@android.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, outreachy-kernel@googlegroups.com Subject: Re: [Outreachy kernel] [PATCH v2] staging: android: Replace strcpy with strlcpy In-Reply-To: Message-ID: References: <20170311211725.GA12884@singhal-Inspiron-5558> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 12 Mar 2017, SIMRAN SINGHAL wrote: > On Sun, Mar 12, 2017 at 2:53 AM, Julia Lawall wrote: > > > > > > On Sun, 12 Mar 2017, 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 > >> --- > >> > >> v2: > >> -Correcting the place of the parenthesis > >> > >> 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 appears to be the old patch. > > > > Actually, looking closer, I don't think this is right at all. There seems > > to be a goal of leaving some space at the beginning of asma->name. So the > > amount of space available for local_name would be computed using a > > subtraction, not an addition. > > > Julia, By mistake I sent the wrong patch. > And I am not getting you want to mention which space. The following is the whole asma->name array. The | in the middle is the offset ASHMEM_NAME_PREFIX_LEN. The xxx characters are in the ASHMEM_NAME_PREFIX_LEN region. The space for local_name is only the part indicates with the ys. This space is smaller than the size of asma->name. In your calculation, you are allowing it to be larger. You will potentially overflow the buffer by 2 * ASHMEM_NAME_PREFIX_LEN. -------------------------- |xxxxxxx|yyyyyyyyyyyyyyyyyy| -------------------------- julia