Message ID | 202212231040562072342@zte.com.cn |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:e747:0:0:0:0:0 with SMTP id c7csp102214wrn; Thu, 22 Dec 2022 18:43:10 -0800 (PST) X-Google-Smtp-Source: AMrXdXscRfTrm7q6LGbMNTAMLXCv/MKANahmvFIv9ybwHqAPskmXwG2iJdF5avWFnmJwh8v6qezZ X-Received: by 2002:aa7:cd04:0:b0:479:7026:b992 with SMTP id b4-20020aa7cd04000000b004797026b992mr6347281edw.38.1671763389942; Thu, 22 Dec 2022 18:43:09 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1671763389; cv=none; d=google.com; s=arc-20160816; b=oJaez1g4mZ/+GFuktb8xakHfd+sbQq/n6OuYbcquSFQNA51uvrNCtnIbJ0zl6cZlys oo8txII7/tOg5jYIKuxKOuKRRQ1inw2ARNsXyb4BRcGv+ErRdt92VPogxSqrSM8cpMe+ ZhAYnC1fmjOQwXQKqqkjNGD2KvkhEwoHjIgU1tdgEJ3q5QIxWK9aGhOZKCrwYj7G41B5 x9x4A/2xl8bElYwd5el32cWGG5EJAzYD7UDfjMHpmj1CZmxeCKEpkmzdANGKZ2op3la7 3nxP4dSFt0RInzdvz+pPLuFJkOFDkgE1sGjRqkKP0tSz7BwyaCMU3GepRAbbVUAPx2JE krqw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:subject:cc:to:from:mime-version:message-id:date; bh=Gsl9vmopu+Sx1tSnvLI7ireI9y0xFBWs4ozEmMSNkH4=; b=Uosv1LltF6+X4hsUguPkupqT6ygw5DBivT4dL5btjWvb6NRCSHNDYWVm4baXaeo+Q3 /thk+9hdwxWUCvucq1RGfXRU1ZCtiGphXUtXYDm5z5cubjPF+Gp0Vn0XFYkt816TW3WJ mgId2qlcB4pPzjtoLQ7CMGjIDI9Jx6qU96+oS1GzGepanllhmCdZPbvp0RIbQF5dY+tP fSUmKxU0lgBAr7u7BeTlpNZp9On+j5sfHE7wC86p2kvrT4vFWooKL/UJqKAr+aPNBBPN JlQWUBV/C8sfGZmB2uu09Nn83c5zQDJHU7rsRnLLIvnZIdByYNWnxaa/MpjE2B6ro9g+ mqyQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=zte.com.cn Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id gt39-20020a1709072da700b0078849a014e9si1928812ejc.196.2022.12.22.18.42.45; Thu, 22 Dec 2022 18:43:09 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=zte.com.cn Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235740AbiLWClE (ORCPT <rfc822;pacteraone@gmail.com> + 99 others); Thu, 22 Dec 2022 21:41:04 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43224 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235374AbiLWClD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 22 Dec 2022 21:41:03 -0500 Received: from mxhk.zte.com.cn (mxhk.zte.com.cn [63.216.63.35]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 938211928A; Thu, 22 Dec 2022 18:41:02 -0800 (PST) Received: from mse-fl1.zte.com.cn (unknown [10.5.228.132]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mxhk.zte.com.cn (FangMail) with ESMTPS id 4NdWdT2kFBz4xVnK; Fri, 23 Dec 2022 10:41:01 +0800 (CST) Received: from szxlzmapp07.zte.com.cn ([10.5.230.251]) by mse-fl1.zte.com.cn with SMTP id 2BN2esCr070573; Fri, 23 Dec 2022 10:40:54 +0800 (+08) (envelope-from yang.yang29@zte.com.cn) Received: from mapi (szxlzmapp02[null]) by mapi (Zmail) with MAPI id mid14; Fri, 23 Dec 2022 10:40:56 +0800 (CST) Date: Fri, 23 Dec 2022 10:40:56 +0800 (CST) X-Zmail-TransId: 2b0463a515387568e84e X-Mailer: Zmail v1.0 Message-ID: <202212231040562072342@zte.com.cn> Mime-Version: 1.0 From: <yang.yang29@zte.com.cn> To: <james.bottomley@hansenpartnership.com> Cc: <deller@gmx.de>, <linux-parisc@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <xu.panda@zte.com.cn>, <yang.yang29@zte.com.cn> Subject: =?utf-8?q?=5BPATCH_linux-next=5D_parisc=3A_use_strscpy=28=29_to_ins?= =?utf-8?q?tead_of_strncpy=28=29?= Content-Type: text/plain; charset="UTF-8" X-MAIL: mse-fl1.zte.com.cn 2BN2esCr070573 X-Fangmail-Gw-Spam-Type: 0 X-FangMail-Miltered: at cgslv5.04-192.168.250.138.novalocal with ID 63A5153D.000 by FangMail milter! X-FangMail-Envelope: 1671763261/4NdWdT2kFBz4xVnK/63A5153D.000/10.5.228.132/[10.5.228.132]/mse-fl1.zte.com.cn/<yang.yang29@zte.com.cn> X-Fangmail-Anti-Spam-Filtered: true X-Fangmail-MID-QID: 63A5153D.000/4NdWdT2kFBz4xVnK X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1752970968079046235?= X-GMAIL-MSGID: =?utf-8?q?1752970968079046235?= |
Series |
[linux-next] parisc: use strscpy() to instead of strncpy()
|
|
Commit Message
Yang Yang
Dec. 23, 2022, 2:40 a.m. UTC
From: Xu Panda <xu.panda@zte.com.cn> The implementation of strscpy() is more robust and safer. That's now the recommended way to copy NUL-terminated strings. Signed-off-by: Xu Panda <xu.panda@zte.com.cn> Signed-off-by: Yang Yang <yang.yang29@zte.com> --- drivers/parisc/pdc_stable.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Comments
On 12/23/22 03:40, yang.yang29@zte.com.cn wrote: > From: Xu Panda <xu.panda@zte.com.cn> > > The implementation of strscpy() is more robust and safer. > That's now the recommended way to copy NUL-terminated strings. Thanks for your patch, but.... > Signed-off-by: Xu Panda <xu.panda@zte.com.cn> > Signed-off-by: Yang Yang <yang.yang29@zte.com> > --- > drivers/parisc/pdc_stable.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c > index d6af5726ddf3..403bca0021c5 100644 > --- a/drivers/parisc/pdc_stable.c > +++ b/drivers/parisc/pdc_stable.c > @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry *entry, const char *buf, size_t coun > > /* We'll use a local copy of buf */ > count = min_t(size_t, count, sizeof(in)-1); > - strncpy(in, buf, count); > - in[count] = '\0'; > + strscpy(in, buf, count + 1); could you resend it somewhat simplified, e.g. strscpy(in, buf, sizeof(in)); > > /* Let's clean up the target. 0xff is a blank pattern */ > memset(&hwpath, 0xff, sizeof(hwpath)); > @@ -388,8 +387,7 @@ pdcspath_layer_write(struct pdcspath_entry *entry, const char *buf, size_t count > > /* We'll use a local copy of buf */ > count = min_t(size_t, count, sizeof(in)-1); > - strncpy(in, buf, count); > - in[count] = '\0'; > + strscpy(in, buf, count + 1); same here > > /* Let's clean up the target. 0 is a blank pattern */ > memset(&layers, 0, sizeof(layers)); > @@ -756,8 +754,7 @@ static ssize_t pdcs_auto_write(struct kobject *kobj, > > /* We'll use a local copy of buf */ > count = min_t(size_t, count, sizeof(in)-1); > - strncpy(in, buf, count); > - in[count] = '\0'; > + strscpy(in, buf, count + 1); and here > > /* Current flags are stored in primary boot path entry */ > pathentry = &pdcspath_entry_primary;
On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote: > On 12/23/22 03:40, yang.yang29@zte.com.cn wrote: > > From: Xu Panda <xu.panda@zte.com.cn> > > > > The implementation of strscpy() is more robust and safer. > > That's now the recommended way to copy NUL-terminated strings. > > Thanks for your patch, but.... > > > Signed-off-by: Xu Panda <xu.panda@zte.com.cn> > > Signed-off-by: Yang Yang <yang.yang29@zte.com> > > --- > > drivers/parisc/pdc_stable.c | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/parisc/pdc_stable.c > > b/drivers/parisc/pdc_stable.c > > index d6af5726ddf3..403bca0021c5 100644 > > --- a/drivers/parisc/pdc_stable.c > > +++ b/drivers/parisc/pdc_stable.c > > @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry > > *entry, const char *buf, size_t coun > > > > /* We'll use a local copy of buf */ > > count = min_t(size_t, count, sizeof(in)-1); > > - strncpy(in, buf, count); > > - in[count] = '\0'; > > + strscpy(in, buf, count + 1); > > could you resend it somewhat simplified, e.g. > strscpy(in, buf, sizeof(in)); I don't think you can: count is the size of buf, if that's < sizeof(in) you've introduced a write beyond end of buffer. In fact sysfs tends to pass pages as buffers, so there's no actual problem, but if that ever changed ... James
Hi James, On 12/27/22 13:38, James Bottomley wrote: > On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote: >> On 12/23/22 03:40, yang.yang29@zte.com.cn wrote: >>> From: Xu Panda <xu.panda@zte.com.cn> >>> >>> The implementation of strscpy() is more robust and safer. >>> That's now the recommended way to copy NUL-terminated strings. >> >> Thanks for your patch, but.... >> >>> Signed-off-by: Xu Panda <xu.panda@zte.com.cn> >>> Signed-off-by: Yang Yang <yang.yang29@zte.com> >>> --- >>> drivers/parisc/pdc_stable.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/parisc/pdc_stable.c >>> b/drivers/parisc/pdc_stable.c >>> index d6af5726ddf3..403bca0021c5 100644 >>> --- a/drivers/parisc/pdc_stable.c >>> +++ b/drivers/parisc/pdc_stable.c >>> @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry >>> *entry, const char *buf, size_t coun >>> >>> /* We'll use a local copy of buf */ >>> count = min_t(size_t, count, sizeof(in)-1); >>> - strncpy(in, buf, count); >>> - in[count] = '\0'; >>> + strscpy(in, buf, count + 1); >> >> could you resend it somewhat simplified, e.g. >> strscpy(in, buf, sizeof(in)); > > I don't think you can: count is the size of buf, if that's < sizeof(in) > you've introduced a write beyond end of buffer. In fact sysfs tends to > pass pages as buffers, so there's no actual problem, but if that ever > changed ... Huh?... he doesn't change "count", so what's wrong with the latest patch? Helge
On Tue, 2022-12-27 at 22:38 +0100, Helge Deller wrote: > Hi James, > > On 12/27/22 13:38, James Bottomley wrote: > > On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote: > > > On 12/23/22 03:40, yang.yang29@zte.com.cn wrote: > > > > From: Xu Panda <xu.panda@zte.com.cn> > > > > > > > > The implementation of strscpy() is more robust and safer. > > > > That's now the recommended way to copy NUL-terminated strings. > > > > > > Thanks for your patch, but.... > > > > > > > Signed-off-by: Xu Panda <xu.panda@zte.com.cn> > > > > Signed-off-by: Yang Yang <yang.yang29@zte.com> > > > > --- > > > > drivers/parisc/pdc_stable.c | 9 +++------ > > > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/parisc/pdc_stable.c > > > > b/drivers/parisc/pdc_stable.c > > > > index d6af5726ddf3..403bca0021c5 100644 > > > > --- a/drivers/parisc/pdc_stable.c > > > > +++ b/drivers/parisc/pdc_stable.c > > > > @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry > > > > *entry, const char *buf, size_t coun > > > > > > > > /* We'll use a local copy of buf */ > > > > count = min_t(size_t, count, sizeof(in)-1); > > > > - strncpy(in, buf, count); > > > > - in[count] = '\0'; > > > > + strscpy(in, buf, count + 1); > > > > > > could you resend it somewhat simplified, e.g. > > > strscpy(in, buf, sizeof(in)); > > > > I don't think you can: count is the size of buf, if that's < > > sizeof(in) you've introduced a write beyond end of buffer. In fact > > sysfs tends to pass pages as buffers, so there's no actual problem, > > but if that ever changed ... > > Huh?... he doesn't change "count", so what's wrong with the latest > patch? the array buf[] is actually buf[count], so if count < 64 then sizeof(buf) < sizeof(in) and you're copying whatever is after buf on the stack or wherever it comes from. The amount you copy into in[] truly has to be the smaller of count and sizeof(in). These are file operations, so you shouldn't rely on buf[] being null terminated (kernfs ensures it is, but it's a dangerous thing to rely on in the face of someone trying to exploit a stack smashing attack). James
> the array buf[] is actually buf[count], so if count < 64 then > sizeof(buf) < sizeof(in) and you're copying whatever is after buf on > the stack or wherever it comes from. The amount you copy into in[] > truly has to be the smaller of count and sizeof(in). These are file > operations, so you shouldn't rely on buf[] being null terminated > (kernfs ensures it is, but it's a dangerous thing to rely on in the > face of someone trying to exploit a stack smashing attack). Should we send patchv3 which is back to v1, or we directly use patchv1 to continue the reviewing? Thanks!
On 12/27/22 23:43, James Bottomley wrote: > On Tue, 2022-12-27 at 22:38 +0100, Helge Deller wrote: >> Hi James, >> >> On 12/27/22 13:38, James Bottomley wrote: >>> On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote: >>>> On 12/23/22 03:40, yang.yang29@zte.com.cn wrote: >>>>> From: Xu Panda <xu.panda@zte.com.cn> >>>>> >>>>> The implementation of strscpy() is more robust and safer. >>>>> That's now the recommended way to copy NUL-terminated strings. >>>> >>>> Thanks for your patch, but.... >>>> >>>>> Signed-off-by: Xu Panda <xu.panda@zte.com.cn> >>>>> Signed-off-by: Yang Yang <yang.yang29@zte.com> >>>>> --- >>>>> drivers/parisc/pdc_stable.c | 9 +++------ >>>>> 1 file changed, 3 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/parisc/pdc_stable.c >>>>> b/drivers/parisc/pdc_stable.c >>>>> index d6af5726ddf3..403bca0021c5 100644 >>>>> --- a/drivers/parisc/pdc_stable.c >>>>> +++ b/drivers/parisc/pdc_stable.c >>>>> @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry >>>>> *entry, const char *buf, size_t coun >>>>> >>>>> /* We'll use a local copy of buf */ >>>>> count = min_t(size_t, count, sizeof(in)-1); >>>>> - strncpy(in, buf, count); >>>>> - in[count] = '\0'; >>>>> + strscpy(in, buf, count + 1); >>>> >>>> could you resend it somewhat simplified, e.g. >>>> strscpy(in, buf, sizeof(in)); >>> >>> I don't think you can: count is the size of buf, if that's < >>> sizeof(in) you've introduced a write beyond end of buffer. In fact >>> sysfs tends to pass pages as buffers, so there's no actual problem, >>> but if that ever changed ... >> >> Huh?... he doesn't change "count", so what's wrong with the latest >> patch? > > the array buf[] is actually buf[count], so if count < 64 then > sizeof(buf) < sizeof(in) and you're copying whatever is after buf on > the stack or wherever it comes from. The amount you copy into in[] > truly has to be the smaller of count and sizeof(in). These are file > operations, so you shouldn't rely on buf[] being null terminated Ok, the main point I missed was that buf[] might not be null terminated. Thanks for the explanation. Yang & Xu, no need to resend the patch. I'll take your v1 version. Thanks! Helge > (kernfs ensures it is, but it's a dangerous thing to rely on in the > face of someone trying to exploit a stack smashing attack). > > James >
diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c index d6af5726ddf3..403bca0021c5 100644 --- a/drivers/parisc/pdc_stable.c +++ b/drivers/parisc/pdc_stable.c @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry *entry, const char *buf, size_t coun /* We'll use a local copy of buf */ count = min_t(size_t, count, sizeof(in)-1); - strncpy(in, buf, count); - in[count] = '\0'; + strscpy(in, buf, count + 1); /* Let's clean up the target. 0xff is a blank pattern */ memset(&hwpath, 0xff, sizeof(hwpath)); @@ -388,8 +387,7 @@ pdcspath_layer_write(struct pdcspath_entry *entry, const char *buf, size_t count /* We'll use a local copy of buf */ count = min_t(size_t, count, sizeof(in)-1); - strncpy(in, buf, count); - in[count] = '\0'; + strscpy(in, buf, count + 1); /* Let's clean up the target. 0 is a blank pattern */ memset(&layers, 0, sizeof(layers)); @@ -756,8 +754,7 @@ static ssize_t pdcs_auto_write(struct kobject *kobj, /* We'll use a local copy of buf */ count = min_t(size_t, count, sizeof(in)-1); - strncpy(in, buf, count); - in[count] = '\0'; + strscpy(in, buf, count + 1); /* Current flags are stored in primary boot path entry */ pathentry = &pdcspath_entry_primary;