Message ID | 20231018-strncpy-drivers-nvdimm-btt-c-v1-1-58070f7dc5c9@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2010:b0:403:3b70:6f57 with SMTP id fe16csp34666vqb; Wed, 18 Oct 2023 15:36:20 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFuBc7cq/qbGttg/CyPaB3AI8ZEbaa+QLOLCS7Yi+g4AmzarfiRzGafIYIJ1Xjw9T1Aos9s X-Received: by 2002:a17:90a:684e:b0:27d:882f:e6cd with SMTP id e14-20020a17090a684e00b0027d882fe6cdmr508533pjm.44.1697668580639; Wed, 18 Oct 2023 15:36:20 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697668580; cv=none; d=google.com; s=arc-20160816; b=k3/sjwUfSzvNZKUuxpxBLeIn7QTOUEmvfqrCFVSGoHst7g5h0CfuMwfPQAkInvgA/L GRDLOSrPxPArxAaWw9SfD2T1MsS+Q03MqDs62007pBp5aGJES3RuLGLg5ZPT2RxGHM39 3l8LzXPvtKD5aG/tD0golXyqhPzxOqY62jl98TUGA2AxtKKUPI5sQUwTrsPtbpZqB2Xw pl2+3RmA/fxCfr/7K9aY3zw79fI47SgoVsOhR2/YuD3ERE64RUNDZl4DmsatqNsRLsyR hWmVAhqJ0INhfumV+1gUxy5klL3C8VKMhu/SEYoU8fFaEFGrz8c2Ro+7SOz3H8Up52fl IYTw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :dkim-signature; bh=U1aLFdy4LtlPdLdmsrELCrrPTI/o77pTc9HEh1RET2I=; fh=XaA6443ioFKS9RZno5StOngvcPVgeZ7i0w5pPdLX5rA=; b=ItViNf0ezrkVgxtbV9WSKCvE1OqUD4ZBTbWnzkr1sCOM59+yie3VAtI3ogGaVx/K0A 96O+Akb5GJ/ho+ySJSuONJqkBWF8OFz3v7tquhXmEAh4IpnJqy3H3N0TJQrjBDFzDdCr EZQ2vgUfOoUvCZMCe815m93+FQ0GS3Guit8STqRg/5x6Wr41RqNK4eWKwpBvpvy0DYhv u4ZJLHpL2NFmK3NBX/STfEnlRU/MuYkpg5iLR8EhhTy8AD/FluslB4w49zH18kN5IyIg zx1Kkvkloml1XF5Nl+zvzTYdGW6Xed6QytChvYsQAKmW+78U2zqFqrvB2i098k+/Inv9 XmQg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=2p2deGNH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id il7-20020a17090b164700b0027b15e572c2si948254pjb.21.2023.10.18.15.36.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 Oct 2023 15:36:20 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=2p2deGNH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id F0EEF8113ECC; Wed, 18 Oct 2023 15:36:16 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230051AbjJRWfw (ORCPT <rfc822;zwp10758@gmail.com> + 24 others); Wed, 18 Oct 2023 18:35:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51840 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229487AbjJRWfu (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 18 Oct 2023 18:35:50 -0400 Received: from mail-yb1-xb4a.google.com (mail-yb1-xb4a.google.com [IPv6:2607:f8b0:4864:20::b4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D59B8113 for <linux-kernel@vger.kernel.org>; Wed, 18 Oct 2023 15:35:48 -0700 (PDT) Received: by mail-yb1-xb4a.google.com with SMTP id 3f1490d57ef6-d9a5a3f2d4fso10179877276.3 for <linux-kernel@vger.kernel.org>; Wed, 18 Oct 2023 15:35:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1697668548; x=1698273348; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=U1aLFdy4LtlPdLdmsrELCrrPTI/o77pTc9HEh1RET2I=; b=2p2deGNHebLT4omQqAuYtJ4sY0mclka+JFK43mbygOfC0S3ELvUQIZJy2zK8/e0xau htu9nWxPiycgejEvsZhGPBgA5GBH27pu1j0qirCXPgQeO1fWl9aLh+qcCiO+x1baE22x jhWuDcbkIM+4occcsxqZ4u4j8E4FQ8BzRZgeL0DBSNkcoF3U55iqT7OvlSMyEaOR1HCF +ixVsEtDtDdPCsoZGath00LU4eXoC/m78Mh6Vz2KWlCEL/vGJtIkOZlk59i+34N46BHc fHExU/qYYf7CEzND76rpM8W/HQ60Iwx+WVghUWB+l+qXf6MDsaj6/bWxCCbW8fnaDQd5 wCfw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697668548; x=1698273348; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=U1aLFdy4LtlPdLdmsrELCrrPTI/o77pTc9HEh1RET2I=; b=dLyXcjRW6ZLeBxY60tB/U9vvsvJNVUipWGHlCYgKCWDh0x0eo3yWKmObGNyon8Zxer Al2thMNvKVM25D3UjYX+A5Cq3Gmll5IYadBJPVuzdD3w6p/03IcKF6+NSyDlwGm69Bpc MB4+YN12P+/VxJIbDqCJ5DPsi8lZm6IfyeKV8cyue2R6IFBZRsqHOQMQrgxVhL+NyGc1 9Ap0kzbL+91fIwmzPMVHtcFWXiYotjKvbeTcZXrsDmO+4vFs+CMQHHQMMzc64Y2Czs0T A1jl4XUNdDDYw0qxWJ+SEPB6R9Rb8LB3Wdwxx7dXZ4WOZgfVap84hfzmObbRwSZZ9M+T uRIg== X-Gm-Message-State: AOJu0YzN9F+3vz0FuNYf+3MQimPvT4OVgbPTKkrgBxmCmoYKZXkjIvRZ 7I5rWX5gSZOKAt7scsbu3eZ6qoAl4CBA+KvuDw== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a25:b0a8:0:b0:d9a:3a14:a5a2 with SMTP id f40-20020a25b0a8000000b00d9a3a14a5a2mr15465ybj.13.1697668548155; Wed, 18 Oct 2023 15:35:48 -0700 (PDT) Date: Wed, 18 Oct 2023 22:35:47 +0000 Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIAMJdMGUC/x3MwQqDMAwA0F+RnA20yljrr4iH2cYth3aSlOIQ/ 92y47u8E5SESWHqThCqrPzNDbbvIHxe+U3IsRkGM4zWWIdaJIf9h1G4kijmGjklXEvBgPax+dE 9vXeeoBW70MbHv5+X67oBjezIGm4AAAA= X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1697668547; l=2405; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=wIXUkCLacDcvyYmWwXYlHGDFUt9BdHJS6MZL7uBj9BA=; b=n6GHlGhf250MWDnVuic43BVfEnftE5cYh9pXrKoHjqf2BnIxPPqvucB7iOWWF8tO9siLtvPfq 0GZIeKqaZp+DOlGpq6Beb706YYhYEaXrCt36mqglIArldQsjrnrBK+2 X-Mailer: b4 0.12.3 Message-ID: <20231018-strncpy-drivers-nvdimm-btt-c-v1-1-58070f7dc5c9@google.com> Subject: [PATCH] block: replace deprecated strncpy with strscpy From: Justin Stitt <justinstitt@google.com> To: Dan Williams <dan.j.williams@intel.com>, Vishal Verma <vishal.l.verma@intel.com>, Dave Jiang <dave.jiang@intel.com>, Ira Weiny <ira.weiny@intel.com> Cc: nvdimm@lists.linux.dev, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, Justin Stitt <justinstitt@google.com> Content-Type: text/plain; charset="utf-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 18 Oct 2023 15:36:17 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780134529533792963 X-GMAIL-MSGID: 1780134529533792963 |
Series |
block: replace deprecated strncpy with strscpy
|
|
Commit Message
Justin Stitt
Oct. 18, 2023, 10:35 p.m. UTC
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
We expect super->signature to be NUL-terminated based on its usage with
memcpy against a NUL-term'd buffer:
btt_devs.c:
253 | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0)
btt.h:
13 | #define BTT_SIG "BTT_ARENA_INFO\0"
NUL-padding is not required as `super` is already zero-allocated:
btt.c:
985 | super = kzalloc(sizeof(struct btt_sb), GFP_NOIO);
... rendering any additional NUL-padding superfluous.
Considering the above, a suitable replacement is `strscpy` [2] due to
the fact that it guarantees NUL-termination on the destination buffer
without unnecessarily NUL-padding.
Let's also use the more idiomatic strscpy usage of (dest, src,
sizeof(dest)) instead of (dest, src, XYZ_LEN) for buffers that the
compiler can determine the size of. This more tightly correlates the
destination buffer to the amount of bytes copied.
Side note, this pattern of memcmp() on two NUL-terminated strings should
really be changed to just a strncmp(), if i'm not mistaken? I see
multiple instances of this pattern in this system.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/nvdimm/btt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231018-strncpy-drivers-nvdimm-btt-c-15f93879989e
Best regards,
--
Justin Stitt <justinstitt@google.com>
Comments
I have a feeling I may have botched the subject line for this patch. Can anyone confirm if it's good or not? Automated tooling told me that this was the most common patch prefix but it may be caused by large patch series that just happened to touch this file once. Should the subject be: nvdimm/btt: ... ? Judging from [1] I see a few "block" and a few of nvdimm/btt. On Wed, Oct 18, 2023 at 3:35 PM Justin Stitt <justinstitt@google.com> wrote: > > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect super->signature to be NUL-terminated based on its usage with > memcpy against a NUL-term'd buffer: > btt_devs.c: > 253 | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) > btt.h: > 13 | #define BTT_SIG "BTT_ARENA_INFO\0" > > NUL-padding is not required as `super` is already zero-allocated: > btt.c: > 985 | super = kzalloc(sizeof(struct btt_sb), GFP_NOIO); > ... rendering any additional NUL-padding superfluous. > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Let's also use the more idiomatic strscpy usage of (dest, src, > sizeof(dest)) instead of (dest, src, XYZ_LEN) for buffers that the > compiler can determine the size of. This more tightly correlates the > destination buffer to the amount of bytes copied. > > Side note, this pattern of memcmp() on two NUL-terminated strings should > really be changed to just a strncmp(), if i'm not mistaken? I see > multiple instances of this pattern in this system. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Note: build-tested only. > > Found with: $ rg "strncpy\(" > --- > drivers/nvdimm/btt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index d5593b0dc700..9372c36e8f76 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -986,7 +986,7 @@ static int btt_arena_write_layout(struct arena_info *arena) > if (!super) > return -ENOMEM; > > - strncpy(super->signature, BTT_SIG, BTT_SIG_LEN); > + strscpy(super->signature, BTT_SIG, sizeof(super->signature)); > export_uuid(super->uuid, nd_btt->uuid); > export_uuid(super->parent_uuid, parent_uuid); > super->flags = cpu_to_le32(arena->flags); > > --- > base-commit: 58720809f52779dc0f08e53e54b014209d13eebb > change-id: 20231018-strncpy-drivers-nvdimm-btt-c-15f93879989e > > Best regards, > -- > Justin Stitt <justinstitt@google.com> > [1]: https://lore.kernel.org/all/?q=dfn%3Adrivers%2Fnvdimm%2Fbtt.c Thanks Justin
On Wed, Oct 18, 2023 at 03:39:59PM -0700, Justin Stitt wrote: > I have a feeling I may have botched the subject line for this patch. > Can anyone confirm if it's good or not? > > Automated tooling told me that this was the most common patch > prefix but it may be caused by large patch series that just > happened to touch this file once. > > Should the subject be: nvdimm/btt: ... ? > > Judging from [1] I see a few "block" and a few of nvdimm/btt. Hi Justin, It should be nvdimm/btt because it only touches btt.c. Here's the old school way that I use to find prefixes. Maybe you can train your automated tooling to do this, and then share it with me ;) I do: ~/git/linux/drivers/nvdimm$ git log --pretty=oneline --abbrev-commit btt.c 3222d8c2a7f8 block: remove ->rw_page ba229aa8f249 nvdimm-btt: Use the enum req_op type 86947df3a923 block: Change the type of the last .rw_page() argument 8b9ab6266204 block: remove blk_cleanup_disk 3205190655ea nvdimm-btt: use bvec_kmap_local in btt_rw_integrity 322cbb50de71 block: remove genhd.h And I see a few choices, with 'block' being pretty common. I peek in those patches and see that block was used when the patch included files in drivers/block AND also in nvdimm/btt. Use nvdimm/btt for your patch. A bit more below - > > On Wed, Oct 18, 2023 at 3:35 PM Justin Stitt <justinstitt@google.com> wrote: > > > > strncpy() is deprecated for use on NUL-terminated destination strings > > [1] and as such we should prefer more robust and less ambiguous string > > interfaces. > > > > We expect super->signature to be NUL-terminated based on its usage with > > memcpy against a NUL-term'd buffer: > > btt_devs.c: > > 253 | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) > > btt.h: > > 13 | #define BTT_SIG "BTT_ARENA_INFO\0" > > > > NUL-padding is not required as `super` is already zero-allocated: > > btt.c: > > 985 | super = kzalloc(sizeof(struct btt_sb), GFP_NOIO); > > ... rendering any additional NUL-padding superfluous. > > > > Considering the above, a suitable replacement is `strscpy` [2] due to > > the fact that it guarantees NUL-termination on the destination buffer > > without unnecessarily NUL-padding. > > > > Let's also use the more idiomatic strscpy usage of (dest, src, > > sizeof(dest)) instead of (dest, src, XYZ_LEN) for buffers that the > > compiler can determine the size of. This more tightly correlates the > > destination buffer to the amount of bytes copied. > > > > Side note, this pattern of memcmp() on two NUL-terminated strings should > > really be changed to just a strncmp(), if i'm not mistaken? I see > > multiple instances of this pattern in this system. I'm not following this note about memcmp() usage. Where is that? > > > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > Link: https://github.com/KSPP/linux/issues/90 > > Cc: linux-hardening@vger.kernel.org > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > Note: build-tested only. > > > > Found with: $ rg "strncpy\(" How you found it goes in the commit log, not below the line. > > --- > > drivers/nvdimm/btt.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > > index d5593b0dc700..9372c36e8f76 100644 > > --- a/drivers/nvdimm/btt.c > > +++ b/drivers/nvdimm/btt.c > > @@ -986,7 +986,7 @@ static int btt_arena_write_layout(struct arena_info *arena) > > if (!super) > > return -ENOMEM; > > > > - strncpy(super->signature, BTT_SIG, BTT_SIG_LEN); > > + strscpy(super->signature, BTT_SIG, sizeof(super->signature)); > > export_uuid(super->uuid, nd_btt->uuid); > > export_uuid(super->parent_uuid, parent_uuid); > > super->flags = cpu_to_le32(arena->flags); > > > > --- > > base-commit: 58720809f52779dc0f08e53e54b014209d13eebb > > change-id: 20231018-strncpy-drivers-nvdimm-btt-c-15f93879989e > > > > Best regards, > > -- > > Justin Stitt <justinstitt@google.com> > > > > [1]: https://lore.kernel.org/all/?q=dfn%3Adrivers%2Fnvdimm%2Fbtt.c > > Thanks > Justin >
On Wed, Oct 18, 2023 at 04:33:29PM -0700, Alison Schofield wrote: > On Wed, Oct 18, 2023 at 03:39:59PM -0700, Justin Stitt wrote: > > I have a feeling I may have botched the subject line for this patch. > > Can anyone confirm if it's good or not? > > > > Automated tooling told me that this was the most common patch > > prefix but it may be caused by large patch series that just > > happened to touch this file once. > > > > Should the subject be: nvdimm/btt: ... ? > > > > Judging from [1] I see a few "block" and a few of nvdimm/btt. > > Hi Justin, > > It should be nvdimm/btt because it only touches btt.c. > > Here's the old school way that I use to find prefixes. Maybe you can > train your automated tooling to do this, and then share it with me ;) > > I do: > > ~/git/linux/drivers/nvdimm$ git log --pretty=oneline --abbrev-commit btt.c > > 3222d8c2a7f8 block: remove ->rw_page > ba229aa8f249 nvdimm-btt: Use the enum req_op type > 86947df3a923 block: Change the type of the last .rw_page() argument > 8b9ab6266204 block: remove blk_cleanup_disk > 3205190655ea nvdimm-btt: use bvec_kmap_local in btt_rw_integrity > 322cbb50de71 block: remove genhd.h > > And I see a few choices, with 'block' being pretty common. > I peek in those patches and see that block was used when the patch > included files in drivers/block AND also in nvdimm/btt. Yeah, this "look into the patch" step is what was missing from the tool[1]. I've just tweaked it to weight based on number of files, and the results are more in line with what I'd expect now. The "top 5" best guesses are now: libnvdimm, btt: nvdimm/btt: nvdimm-btt: libnvdimm/btt: block: [1] https://github.com/kees/kernel-tools/blob/trunk/helpers/get-prefix > Use nvdimm/btt for your patch. > > A bit more below - > > > > > On Wed, Oct 18, 2023 at 3:35 PM Justin Stitt <justinstitt@google.com> wrote: > > > > > > strncpy() is deprecated for use on NUL-terminated destination strings > > > [1] and as such we should prefer more robust and less ambiguous string > > > interfaces. > > > > > > We expect super->signature to be NUL-terminated based on its usage with > > > memcpy against a NUL-term'd buffer: typo memcpy -> memcmp above? > > > btt_devs.c: > > > 253 | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) > > > btt.h: > > > 13 | #define BTT_SIG "BTT_ARENA_INFO\0" 1234567890123456 That's a funny way to define a string. :) Now it has two %NULs at the end. ;) It doesn't need that trailing '\0' #define BTT_SIG_LEN 16 And then this could be: #define BTT_SIG_LEN sizeof(BTT_SIG) But anyway... > > > > > > NUL-padding is not required as `super` is already zero-allocated: > > > btt.c: > > > 985 | super = kzalloc(sizeof(struct btt_sb), GFP_NOIO); > > > ... rendering any additional NUL-padding superfluous. > > > > > > Considering the above, a suitable replacement is `strscpy` [2] due to > > > the fact that it guarantees NUL-termination on the destination buffer > > > without unnecessarily NUL-padding. > > > > > > Let's also use the more idiomatic strscpy usage of (dest, src, > > > sizeof(dest)) instead of (dest, src, XYZ_LEN) for buffers that the > > > compiler can determine the size of. This more tightly correlates the > > > destination buffer to the amount of bytes copied. > > > > > > Side note, this pattern of memcmp() on two NUL-terminated strings should > > > really be changed to just a strncmp(), if i'm not mistaken? I see > > > multiple instances of this pattern in this system. > > I'm not following this note about memcmp() usage. Where is that? > > > > > > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > > > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > > Link: https://github.com/KSPP/linux/issues/90 > > > Cc: linux-hardening@vger.kernel.org > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > > --- > > > Note: build-tested only. > > > > > > Found with: $ rg "strncpy\(" > > How you found it goes in the commit log, not below the line. > > > > --- > > > drivers/nvdimm/btt.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > > > index d5593b0dc700..9372c36e8f76 100644 > > > --- a/drivers/nvdimm/btt.c > > > +++ b/drivers/nvdimm/btt.c > > > @@ -986,7 +986,7 @@ static int btt_arena_write_layout(struct arena_info *arena) > > > if (!super) > > > return -ENOMEM; > > > > > > - strncpy(super->signature, BTT_SIG, BTT_SIG_LEN); > > > + strscpy(super->signature, BTT_SIG, sizeof(super->signature)); Yup, this looks right to me. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
On Wed, Oct 18, 2023 at 4:33 PM Alison Schofield <alison.schofield@intel.com> wrote: > > On Wed, Oct 18, 2023 at 03:39:59PM -0700, Justin Stitt wrote: > > I have a feeling I may have botched the subject line for this patch. > > Can anyone confirm if it's good or not? > > > > Automated tooling told me that this was the most common patch > > prefix but it may be caused by large patch series that just > > happened to touch this file once. > > > > Should the subject be: nvdimm/btt: ... ? > > > > Judging from [1] I see a few "block" and a few of nvdimm/btt. > > Hi Justin, > > It should be nvdimm/btt because it only touches btt.c. Got it. I just sent a [v2]. > > Here's the old school way that I use to find prefixes. Maybe you can > train your automated tooling to do this, and then share it with me ;) I use a gently modified version of [1] which I've hardwired into my b4 installation to automatically set the prefix when creating a patch. > > I do: > > ~/git/linux/drivers/nvdimm$ git log --pretty=oneline --abbrev-commit btt.c > > 3222d8c2a7f8 block: remove ->rw_page > ba229aa8f249 nvdimm-btt: Use the enum req_op type > 86947df3a923 block: Change the type of the last .rw_page() argument > 8b9ab6266204 block: remove blk_cleanup_disk > 3205190655ea nvdimm-btt: use bvec_kmap_local in btt_rw_integrity > 322cbb50de71 block: remove genhd.h > > And I see a few choices, with 'block' being pretty common. > I peek in those patches and see that block was used when the patch > included files in drivers/block AND also in nvdimm/btt. > > Use nvdimm/btt for your patch. > > A bit more below - > > > > > On Wed, Oct 18, 2023 at 3:35 PM Justin Stitt <justinstitt@google.com> wrote: > > > > > > strncpy() is deprecated for use on NUL-terminated destination strings > > > [1] and as such we should prefer more robust and less ambiguous string > > > interfaces. > > > > > > We expect super->signature to be NUL-terminated based on its usage with > > > memcpy against a NUL-term'd buffer: > > > btt_devs.c: > > > 253 | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) > > > btt.h: > > > 13 | #define BTT_SIG "BTT_ARENA_INFO\0" > > > > > > NUL-padding is not required as `super` is already zero-allocated: > > > btt.c: > > > 985 | super = kzalloc(sizeof(struct btt_sb), GFP_NOIO); > > > ... rendering any additional NUL-padding superfluous. > > > > > > Considering the above, a suitable replacement is `strscpy` [2] due to > > > the fact that it guarantees NUL-termination on the destination buffer > > > without unnecessarily NUL-padding. > > > > > > Let's also use the more idiomatic strscpy usage of (dest, src, > > > sizeof(dest)) instead of (dest, src, XYZ_LEN) for buffers that the > > > compiler can determine the size of. This more tightly correlates the > > > destination buffer to the amount of bytes copied. > > > > > > Side note, this pattern of memcmp() on two NUL-terminated strings should > > > really be changed to just a strncmp(), if i'm not mistaken? I see > > > multiple instances of this pattern in this system. > > I'm not following this note about memcmp() usage. Where is that? Sorry, I wasn't very clear. I've added more info in [v2] but tl;dr is that it seems weird to me to use memcmp() on two NUL-terminated strings when we have something like strncmp() or strcmp() for that purpose. See [2]. > > > > > > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > > > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > > Link: https://github.com/KSPP/linux/issues/90 > > > Cc: linux-hardening@vger.kernel.org > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > > --- > > > Note: build-tested only. > > > > > > Found with: $ rg "strncpy\(" > > How you found it goes in the commit log, not below the line. Whoops, fixed in [v2]. > > > > --- > > > drivers/nvdimm/btt.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > > > index d5593b0dc700..9372c36e8f76 100644 > > > --- a/drivers/nvdimm/btt.c > > > +++ b/drivers/nvdimm/btt.c > > > @@ -986,7 +986,7 @@ static int btt_arena_write_layout(struct arena_info *arena) > > > if (!super) > > > return -ENOMEM; > > > > > > - strncpy(super->signature, BTT_SIG, BTT_SIG_LEN); > > > + strscpy(super->signature, BTT_SIG, sizeof(super->signature)); > > > export_uuid(super->uuid, nd_btt->uuid); > > > export_uuid(super->parent_uuid, parent_uuid); > > > super->flags = cpu_to_le32(arena->flags); > > > > > > --- > > > base-commit: 58720809f52779dc0f08e53e54b014209d13eebb > > > change-id: 20231018-strncpy-drivers-nvdimm-btt-c-15f93879989e > > > > > > Best regards, > > > -- > > > Justin Stitt <justinstitt@google.com> > > > > > > > [1]: https://lore.kernel.org/all/?q=dfn%3Adrivers%2Fnvdimm%2Fbtt.c > > > > Thanks > > Justin > > [v2]: https://lore.kernel.org/r/20231019-strncpy-drivers-nvdimm-btt-c-v2-1-366993878cf0@google.com [1] https://github.com/kees/kernel-tools/blob/trunk/helpers/get-prefix [2]: https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/nvdimm/btt_devs.c#L253 Thanks Justin
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index d5593b0dc700..9372c36e8f76 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -986,7 +986,7 @@ static int btt_arena_write_layout(struct arena_info *arena) if (!super) return -ENOMEM; - strncpy(super->signature, BTT_SIG, BTT_SIG_LEN); + strscpy(super->signature, BTT_SIG, sizeof(super->signature)); export_uuid(super->uuid, nd_btt->uuid); export_uuid(super->parent_uuid, parent_uuid); super->flags = cpu_to_le32(arena->flags);