From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750891AbdE1SMr (ORCPT ); Sun, 28 May 2017 14:12:47 -0400 Received: from mail.skyhub.de ([5.9.137.197]:39396 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750797AbdE1SMq (ORCPT ); Sun, 28 May 2017 14:12:46 -0400 Date: Sun, 28 May 2017 20:12:31 +0200 From: Borislav Petkov To: Karim Eshapa Cc: mchehab@kernel.org, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] edac: edac_mc_sysfs.c: Use strlcpy to Copy a C-string into a sized buffer Message-ID: <20170528181231.fnhsih22co6bofr3@pd.tnic> References: <1495825821-5295-1-git-send-email-karim.eshapa@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1495825821-5295-1-git-send-email-karim.eshapa@gmail.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 26, 2017 at 09:10:21PM +0200, Karim Eshapa wrote: > Use strlcpy with sized buffer instead of strncpy to > avoid non NUL-terminated source strings problems. Huh? But it does get terminated. > Signed-off-by: Karim Eshapa > --- > drivers/edac/edac_mc_sysfs.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c > index 445862d..e893b94 100644 > --- a/drivers/edac/edac_mc_sysfs.c > +++ b/drivers/edac/edac_mc_sysfs.c > @@ -251,8 +251,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, > if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) > return -EINVAL; > > - strncpy(rank->dimm->label, data, copy_count); > - rank->dimm->label[copy_count] = '\0'; > + strlcpy(rank->dimm->label, data, copy_count); > > return count; > } > @@ -530,8 +529,7 @@ static ssize_t dimmdev_label_store(struct device *dev, > if (copy_count == 0 || copy_count >= sizeof(dimm->label)) > return -EINVAL; > > - strncpy(dimm->label, data, copy_count); > - dimm->label[copy_count] = '\0'; > + strlcpy(dimm->label, data, copy_count); Did you try your patch before sending it? # cat /sys/devices/system/edac/mc/mc0/dimm0/dimm_label CPU_SrcID#0_Ha#0_Chan#0_DIMM#0 # echo "bla" > /sys/devices/system/edac/mc/mc0/dimm0/dimm_label # cat /sys/devices/system/edac/mc/mc0/dimm0/dimm_label bl # echo "bla boom" > /sys/devices/system/edac/mc/mc0/dimm0/dimm_label # cat /sys/devices/system/edac/mc/mc0/dimm0/dimm_label bla boo Also, I don't like the fact that strlcpy does not pad the remainder of the dest buffer with NUL. So unless you're seeing a real bug, please don't waste your time with this. There are enough real bugs in the kernel which need fixing. Thanks. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.