From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752404AbbIXTJB (ORCPT ); Thu, 24 Sep 2015 15:09:01 -0400 Received: from g1t6223.austin.hp.com ([15.73.96.124]:39451 "EHLO g1t6223.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751044AbbIXTI7 (ORCPT ); Thu, 24 Sep 2015 15:08:59 -0400 Message-ID: <1443121564.25474.160.camel@hpe.com> Subject: Re: [PATCH v2 2/2] EDAC: Fix sysfs dimm_label store operation From: Toshi Kani To: Borislav Petkov Cc: mchehab@osg.samsung.com, dougthompson@xmission.com, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org, elliott@hpe.com, tony.luck@intel.com Date: Thu, 24 Sep 2015 13:06:04 -0600 In-Reply-To: <20150924164830.GF3774@pd.tnic> References: <1442933883-21587-1-git-send-email-toshi.kani@hpe.com> <1442933883-21587-3-git-send-email-toshi.kani@hpe.com> <20150924164830.GF3774@pd.tnic> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.5 (3.16.5-1.fc22) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2015-09-24 at 18:48 +0200, Borislav Petkov wrote: > On Tue, Sep 22, 2015 at 08:58:03AM -0600, Toshi Kani wrote: > > Sysfs "dimm_label" and "chX_dimm_label" have the following issues > > in their store operation. > > > > 1) A newline-terminated input string causes redundant newlines > > > > # echo "test" > /sys/bus/mc0/devices/dimm0/dimm_label > > # cat /sys/bus/mc0/devices/dimm0/dimm_label > > test > > > > # od -bc /sys/bus/mc0/devices/dimm0/dimm_label > > 0000000 164 145 163 164 012 012 > > t e s t \n \n > > 0000006 > > > > 2) The original label string (31 characters) cannot be stored due to > > an improper size check > > > > # echo "CPU_SrcID#0_Ha#0_Chan#0_DIMM#0" \ > > > /sys/bus/mc0/devices/dimm0/dimm_label > > # cat /sys/bus/mc0/devices/dimm0/dimm_label > > > > > > # od -bc /sys/bus/mc0/devices/dimm0/dimm_label > > 0000000 012 012 > > \n \n > > 0000002 > > > > 3) An input string longer than the buffer size results a wrong label > > info as it allows a retry with the remaining string. > > > > # echo "CPU_SrcID#0_Ha#0_Chan#0_DIMM#0_TEST" \ > > > /sys/bus/mc0/devices/dimm0/dimm_label > > # cat /sys/bus/mc0/devices/dimm0/dimm_label > > _TEST > > > > Fix these issues by making the following changes: > > 1) Replace a newline charactor at the end by setting a null. It also > > assures that the string is null-terminated within the size. > > 2) Check the label buffer size with 'sizeof(dimm->label)'. > > 3) Fail a request if its string exceeds the label buffer size. > > > > Signed-off-by: Toshi Kani > > Acked-by: Tony Luck > > Cc: Mauro Carvalho Chehab > > Cc: Borislav Petkov > > Cc: Doug Thompson > > Cc: Robert Elliott > > Cc: Tony Luck > > --- > > drivers/edac/edac_mc_sysfs.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > ... > > > @@ -495,13 +495,13 @@ static ssize_t dimmdev_label_store(struct device *dev, > > { > > struct dimm_info *dimm = to_dimm(dev); > > > > - ssize_t max_size = 0; > > + if (count == 0 || count > sizeof(dimm->label)) > > + return -EINVAL; > > $ echo "" > /sys/bus/mc0/devices/dimm0/dimm_label > $ od -bc /sys/bus/mc0/devices/dimm0/dimm_label > 0000000 > $ cat /sys/bus/mc0/devices/dimm0/dimm_label > $ > > I don't think we want to allow empty labels. I guess something like this > (2 because there's also the additional "\n"): edac-utils(1) checks empty labels and shows them as "ch%d" [1]. So, I think empty labels are supported today, and using 'echo "" >' seems to be a legitimate way to set them empty if desired. While looking at the count check, though, I noticed that 'echo -n' does not send a null-terminated string (hence, the count check needs to take it into account) since it simply calls putchar() to send '1' only in the example below. [2] $ echo -n "1" | wc 0 1 1 Attached is updated patch that handles the 'echo -n' case properly. When an input string is not null-terminated, it appends '\0' to the end as long as it fits into the label buffer. Thanks, -Toshi [1] https://github.com/grondo/edac-utils/search?utf8=%E2%9C%93&q=dimm_label_valid&type=Code [2] http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/echo.c ====== Subject: [PATCH v2 UPDATE 2/2] EDAC: Fix sysfs dimm_label store operation Sysfs "dimm_label" and "chX_dimm_label" have the following issues in their store operation. 1) A newline-terminated input string causes redundant newlines # echo "test" > /sys/bus/mc0/devices/dimm0/dimm_label # cat /sys/bus/mc0/devices/dimm0/dimm_label test # od -bc /sys/bus/mc0/devices/dimm0/dimm_label 0000000 164 145 163 164 012 012 t e s t \n \n 0000006 2) The original label string (31 characters) cannot be stored due to an improper size check # echo "CPU_SrcID#0_Ha#0_Chan#0_DIMM#0" \ > /sys/bus/mc0/devices/dimm0/dimm_label # cat /sys/bus/mc0/devices/dimm0/dimm_label # od -bc /sys/bus/mc0/devices/dimm0/dimm_label 0000000 012 012 \n \n 0000002 3) An input string longer than the buffer size results a wrong label info as it allows a retry with the remaining string. # echo "CPU_SrcID#0_Ha#0_Chan#0_DIMM#0_TEST" \ > /sys/bus/mc0/devices/dimm0/dimm_label # cat /sys/bus/mc0/devices/dimm0/dimm_label _TEST Fix these issues by making the following changes: 1) Replace a newline character at the end by setting a null. It also assures that the string is null-terminated in the label buffer. 2) Check the label buffer size with 'sizeof(dimm->label)'. 3) Fail a request if its string exceeds the label buffer size. Signed-off-by: Toshi Kani Acked-by: Tony Luck Cc: Mauro Carvalho Chehab Cc: Borislav Petkov Cc: Doug Thompson Cc: Robert Elliott Cc: Tony Luck --- drivers/edac/edac_mc_sysfs.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 8983755..5252fb9 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -240,14 +240,21 @@ static ssize_t channel_dimm_label_store(struct device *dev, struct csrow_info *csrow = to_csrow(dev); unsigned chan = to_channel(mattr); struct rank_info *rank = csrow->channels[chan]; + size_t copy_count = count; - ssize_t max_size = 0; + if (count == 0) + return -EINVAL; + + if (data[count - 1] == '\0' || data[count - 1] == '\n') + copy_count -= 1; - max_size = min((ssize_t) count, (ssize_t) EDAC_MC_LABEL_LEN - 1); - strncpy(rank->dimm->label, data, max_size); - rank->dimm->label[max_size] = '\0'; + if (copy_count >= sizeof(rank->dimm->label)) + return -EINVAL; - return max_size; + strncpy(rank->dimm->label, data, copy_count); + rank->dimm->label[copy_count] = '\0'; + + return count; } /* show function for dynamic chX_ce_count attribute */ @@ -494,14 +501,21 @@ static ssize_t dimmdev_label_store(struct device *dev, size_t count) { struct dimm_info *dimm = to_dimm(dev); + size_t copy_count = count; - ssize_t max_size = 0; + if (count == 0) + return -EINVAL; + + if (data[count - 1] == '\0' || data[count - 1] == '\n') + copy_count -= 1; - max_size = min((ssize_t) count, (ssize_t) EDAC_MC_LABEL_LEN - 1); - strncpy(dimm->label, data, max_size); - dimm->label[max_size] = '\0'; + if (copy_count >= sizeof(dimm->label)) + return -EINVAL; - return max_size; + strncpy(dimm->label, data, copy_count); + dimm->label[copy_count] = '\0'; + + return count; } static ssize_t dimmdev_size_show(struct device *dev,