Message ID | 20230717093332.54236-1-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c923:0:b0:3e4:2afc:c1 with SMTP id j3csp1006067vqt; Mon, 17 Jul 2023 02:49:40 -0700 (PDT) X-Google-Smtp-Source: APBJJlGZvm1lurb1GhuDoe1wnmwKAUfW6MdfrIs1OJJmkWXDVDxW0BYglgNuCkeuDd28EMzHO+eC X-Received: by 2002:aa7:cd04:0:b0:51b:ec90:cf0f with SMTP id b4-20020aa7cd04000000b0051bec90cf0fmr10458199edw.6.1689587379941; Mon, 17 Jul 2023 02:49:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689587379; cv=none; d=google.com; s=arc-20160816; b=eybp0BNg9zdO93P+Qjwsg0hnfukP0DayySRyoVeNI986Gcn6IpDxgNe5soed5JJegY YSht138TBgdEZkgvL3IVB5eLDG96neoT3r6xascotp45vzfHWofOVWQLEMicp2ErShkX F716JSYBiAI2coXRyMiN05m4InlBE0rFklkdAWg6ALxFcNYIxKztuaFc2j6Eurb0wzba A7syfkBIyUZiAa0sWU94MSvp6d0SjBK9fqURymBETZMhJfuO+G/jYLuHV441oy5XiGWk 3SkhyiKdq85zAtk5vZB3ekSRQEkfFOyyJczhkNq41WvJZxwUwWRrYJTB/CBNVSCrjXgg xJFA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=fAgvyvNC6bc5lck6fZw6TpsQw+wIu3m++3GgDkOrXrM=; fh=C6DoGaVv7Fv6uzcBMQTV/6CFeUaFyPzNyP+cqN6aTZs=; b=doqVz5UTGW2lhO/KigYTaKCozuAbriv2k9ZZI4QbB0g+P1RnsP2aGw7epS7GJCdxXc puZOKjIki9GAnSG8uL0v2s9K74ASOZuYAgK0x9m9Sf3d6Q6gMsA8nGDzzNyVpXDuuL7o 7CItjXx+AicRFkCF8cmUAf2SkRPlXDArI1mpMgSzU+g+VDTi9XbhODN/vv7wzNlCYuGK nJqctayYZBV2QqNwA9L1QuYbJlwYt9ZAKbcen6ZMXZfyN47cXzYK31suYwCgx4ncXA0F SeDoLNE7R73YQneNjv6Bufwf6GvN73mY6lCzUmdvMb+Nt5HwK7jXUDhc3U4a+PChnyVX VpeQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=ObVqPVNS; 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=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b22-20020aa7dc16000000b0051dd3d4d029si5704614edu.510.2023.07.17.02.49.15; Mon, 17 Jul 2023 02:49:39 -0700 (PDT) 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; dkim=pass header.i=@intel.com header.s=Intel header.b=ObVqPVNS; 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=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230354AbjGQJd6 (ORCPT <rfc822;hadasmailinglist@gmail.com> + 99 others); Mon, 17 Jul 2023 05:33:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40584 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230301AbjGQJdx (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 17 Jul 2023 05:33:53 -0400 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1081210E5 for <linux-kernel@vger.kernel.org>; Mon, 17 Jul 2023 02:33:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1689586432; x=1721122432; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=s6ZoXKzWeESKEZrwLJVnimopWYHbgY2dHEYUngYjQ1o=; b=ObVqPVNSNcuyM6qfN2gg1C6ccgc6XF0TguL8UhexP6j6wDLVPiIRADCp /U+llFYrsot4SiIv0DXpdmJg0GfbuY560TkM3ERz3bwEkRtjSg3UKjAR4 rHDmH4vbT8rDkvDzznuAjM9SgqIs+R93oUHUYrqJMakMC4Fdtt36+vxy4 t1kOM+4l5ll1NI9wMGGnpWPVthAmAdETVLta8uJDOazPhC+0+DM9IA/L7 p3IG4vWFqwdwUo8xsFz6plpAJQK4bfya4FRHJdN/ftCQy1swaj+iGTZMG cVqyTA6w2xJrINmRG9F+RR9MmWtLiteSp14GNlCZI9SRBw0t/1XBfsEZv A==; X-IronPort-AV: E=McAfee;i="6600,9927,10773"; a="364762671" X-IronPort-AV: E=Sophos;i="6.01,211,1684825200"; d="scan'208";a="364762671" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Jul 2023 02:33:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10773"; a="847218975" X-IronPort-AV: E=Sophos;i="6.01,211,1684825200"; d="scan'208";a="847218975" Received: from black.fi.intel.com ([10.237.72.28]) by orsmga004.jf.intel.com with ESMTP; 17 Jul 2023 02:33:28 -0700 Received: by black.fi.intel.com (Postfix, from userid 1003) id E0232256; Mon, 17 Jul 2023 12:33:34 +0300 (EEST) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, linux-kernel@vger.kernel.org Cc: Kees Cook <keescook@chromium.org> Subject: [PATCH v1 1/1] seq_file: Replace strncpy()+nul by strscpy() Date: Mon, 17 Jul 2023 12:33:32 +0300 Message-Id: <20230717093332.54236-1-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.40.0.1.gaa8946217a0b MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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: INBOX X-GMAIL-THRID: 1771660776650985939 X-GMAIL-MSGID: 1771660776650985939 |
Series |
[v1,1/1] seq_file: Replace strncpy()+nul by strscpy()
|
|
Commit Message
Andy Shevchenko
July 17, 2023, 9:33 a.m. UTC
Privided seq_show_option_n() macro breaks build with -Werror
and W=1, e.g.:
In function ‘strncpy’,
inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3:
include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation]
68 | #define __underlying_strncpy __builtin_strncpy
| ^
include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’
151 | return __underlying_strncpy(p, q, size);
| ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
While -Werror wasn't enabled by default at the time of the original code
landed into mainline, strscpy() was already there and preferred over strncpy().
Due to above mentioned issues, use the latter in seq_show_option_n().
Fixes: a068acf2ee77 ("fs: create and use seq_show_option for escaping")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
include/linux/seq_file.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Comments
On Mon, Jul 17, 2023 at 12:33:32PM +0300, Andy Shevchenko wrote: > Privided seq_show_option_n() macro breaks build with -Werror > and W=1, e.g.: > > In function ‘strncpy’, > inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3: > include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation] > 68 | #define __underlying_strncpy __builtin_strncpy > | ^ While I totally agree with the removal of strncpy(), I'm confused about how this warning is being produced: seq_show_option_n(s, "cluster_stack", osb->osb_cluster_stack, OCFS2_STACK_LABEL_LEN); fs/ocfs2/ocfs2.h:389: char osb_cluster_stack[OCFS2_STACK_LABEL_LEN + 1]; fs/ocfs2/ocfs2_fs.h:#define OCFS2_STACK_LABEL_LEN 4 #define seq_show_option_n(m, name, value, length) { \ char val_buf[length + 1]; \ strncpy(val_buf, value, length); \ ... the source buffer is OCFS2_STACK_LABEL_LEN + 1 long, and the dest buffer is OCFS2_STACK_LABEL_LEN + 1 long. ?? I think this doesn't need to use seq_show_option_n() at all. > include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’ > 151 | return __underlying_strncpy(p, q, size); > | ^~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > While -Werror wasn't enabled by default at the time of the original code > landed into mainline, strscpy() was already there and preferred over strncpy(). > Due to above mentioned issues, use the latter in seq_show_option_n(). > > Fixes: a068acf2ee77 ("fs: create and use seq_show_option for escaping") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/seq_file.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h > index bd023dd38ae6..e87d635ca24f 100644 > --- a/include/linux/seq_file.h > +++ b/include/linux/seq_file.h > @@ -260,8 +260,7 @@ static inline void seq_show_option(struct seq_file *m, const char *name, > */ > #define seq_show_option_n(m, name, value, length) { \ > char val_buf[length + 1]; \ > - strncpy(val_buf, value, length); \ > - val_buf[length] = '\0'; \ > + strscpy(val_buf, value, sizeof(val_buf)); \ > seq_show_option(m, name, val_buf); \ > } Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
On Mon, Jul 17, 2023 at 08:43:55AM -0700, Kees Cook wrote: > On Mon, Jul 17, 2023 at 12:33:32PM +0300, Andy Shevchenko wrote: ... > I think this doesn't need to use seq_show_option_n() at all. Quite likely. Nevertheless, it's one of the dozens (?) warnings like this. ... > Reviewed-by: Kees Cook <keescook@chromium.org> Thank you for the review! I think it's you who may take it as seq_file.h seems everybody's playground.
On Mon, Jul 17, 2023 at 07:05:44PM +0300, Andy Shevchenko wrote: > On Mon, Jul 17, 2023 at 08:43:55AM -0700, Kees Cook wrote: > > On Mon, Jul 17, 2023 at 12:33:32PM +0300, Andy Shevchenko wrote: > > ... > > > I think this doesn't need to use seq_show_option_n() at all. > > Quite likely. Nevertheless, it's one of the dozens (?) warnings like this. > > ... > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Thank you for the review! > > I think it's you who may take it as seq_file.h seems everybody's playground. Ah, good point. :P
On Mon, 17 Jul 2023 12:33:32 +0300, Andy Shevchenko wrote: > Privided seq_show_option_n() macro breaks build with -Werror > and W=1, e.g.: > > In function ‘strncpy’, > inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3: > include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation] > 68 | #define __underlying_strncpy __builtin_strncpy > | ^ > include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’ > 151 | return __underlying_strncpy(p, q, size); > | ^~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > [...] Applied, thanks! [1/1] seq_file: Replace strncpy()+nul by strscpy() https://git.kernel.org/kees/c/c30417b20f49 Best regards,
From: Andy Shevchenko > Sent: 17 July 2023 10:34 ... > #define seq_show_option_n(m, name, value, length) { \ > char val_buf[length + 1]; \ > - strncpy(val_buf, value, length); \ > - val_buf[length] = '\0'; \ > + strscpy(val_buf, value, sizeof(val_buf)); \ > seq_show_option(m, name, val_buf); \ > } Why the extra double-copy with (potentially) a VLA? seq_show_option() calls seq_escape_str(), seq_escape_str calls seq_escape_mem(... strlen(src) ...). Implement seq_show_option() as seq_show_option_n(... strlen(value)) and use seq_escape_mem() for the value. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Jul 17, 2023 at 04:09:23PM -0700, Kees Cook wrote: > > On Mon, 17 Jul 2023 12:33:32 +0300, Andy Shevchenko wrote: > > Privided seq_show_option_n() macro breaks build with -Werror > > and W=1, e.g.: > > > > In function ‘strncpy’, > > inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3: > > include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation] > > 68 | #define __underlying_strncpy __builtin_strncpy > > | ^ > > include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’ > > 151 | return __underlying_strncpy(p, q, size); > > | ^~~~~~~~~~~~~~~~~~~~ > > cc1: all warnings being treated as errors > > > > [...] > > Applied, thanks! > > [1/1] seq_file: Replace strncpy()+nul by strscpy() > https://git.kernel.org/kees/c/c30417b20f49 Gah, I dropped this from my tree since it was actually wrong[1]. This is an ugly corner case with strscpy vs strncpy: the cast be32 from hfs/hfsplus[2] looks unterminated to strscpy, so it would return -E2BIG, but really FORTIFY noticed the over-read (strscpy is correctly checking the 5th byte for NUL). So... I think we need to fix seq_show_option_n() using memcpy+NUL, drop the ocfs2 usage, and clarify that the seq_show_option_n() docs mean "n means _exactly_ n bytes"... -Kees [1] https://lore.kernel.org/lkml/0000000000000a88cb0600ccef54@google.com/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/hfsplus/options.c?h=v6.4#n221
On Tue, Jul 18, 2023 at 10:00:24PM -0700, Kees Cook wrote: > On Mon, Jul 17, 2023 at 04:09:23PM -0700, Kees Cook wrote: > > On Mon, 17 Jul 2023 12:33:32 +0300, Andy Shevchenko wrote: > > > Privided seq_show_option_n() macro breaks build with -Werror > > > and W=1, e.g.: > > > > > > In function ‘strncpy’, > > > inlined from ‘ocfs2_show_options’ at fs/ocfs2/super.c:1520:3: > > > include/linux/fortify-string.h:68:33: error: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 4 [-Werror=stringop-truncation] > > > 68 | #define __underlying_strncpy __builtin_strncpy > > > | ^ > > > include/linux/fortify-string.h:151:16: note: in expansion of macro ‘__underlying_strncpy’ > > > 151 | return __underlying_strncpy(p, q, size); > > > | ^~~~~~~~~~~~~~~~~~~~ > > > cc1: all warnings being treated as errors > > > > > > [...] > > > > Applied, thanks! > > > > [1/1] seq_file: Replace strncpy()+nul by strscpy() > > https://git.kernel.org/kees/c/c30417b20f49 > > Gah, I dropped this from my tree since it was actually wrong[1]. This is an > ugly corner case with strscpy vs strncpy: the cast be32 from hfs/hfsplus[2] > looks unterminated to strscpy, so it would return -E2BIG, but really > FORTIFY noticed the over-read (strscpy is correctly checking the 5th > byte for NUL). > > So... I think we need to fix seq_show_option_n() using memcpy+NUL, drop > the ocfs2 usage, and clarify that the seq_show_option_n() docs mean > "n means _exactly_ n bytes"... Sounds like a plan! Please go ahead with that. My intention is to make eventually build kernel with `make W=1` when CONFIG_WERROR=y. > [1] https://lore.kernel.org/lkml/0000000000000a88cb0600ccef54@google.com/ > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/hfsplus/options.c?h=v6.4#n221
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h index bd023dd38ae6..e87d635ca24f 100644 --- a/include/linux/seq_file.h +++ b/include/linux/seq_file.h @@ -260,8 +260,7 @@ static inline void seq_show_option(struct seq_file *m, const char *name, */ #define seq_show_option_n(m, name, value, length) { \ char val_buf[length + 1]; \ - strncpy(val_buf, value, length); \ - val_buf[length] = '\0'; \ + strscpy(val_buf, value, sizeof(val_buf)); \ seq_show_option(m, name, val_buf); \ }