* [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source
@ 2023-08-28 22:25 Nick Desaulniers
2023-08-28 22:25 ` [PATCH 1/2] scsi: myrb: fix -Wfortify-source Nick Desaulniers
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Nick Desaulniers @ 2023-08-28 22:25 UTC (permalink / raw)
To: Hannes Reinecke, James E.J. Bottomley, Martin K. Petersen,
Nathan Chancellor
Cc: Tom Rix, linux-scsi, linux-kernel, llvm, Nick Desaulniers
clang-18 has improved its support for detecting operations that will
truncate values at runtime via -wfortify-source resulting in two new
warnings (or errors with CONFIG_WERROR=y):
drivers/scsi/myrb.c:1906:10: warning: 'snprintf' will always be
truncated; specified size is 32, but format string expands to at least
34 [-Wfortify-source]
drivers/scsi/myrs.c:1089:10: warning: 'snprintf' will always be
truncated; specified size is 32, but format string expands to at least
34 [-Wfortify-source]
When we have a string literal that does not contain any format flags,
rather than use snprintf (sometimes with a size that's too small), let's
use sprintf.
This is pattern is cleaned up throughout two files.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Nick Desaulniers (2):
scsi: myrb: fix -Wfortify-source
scsi: myrs: fix -Wfortify-source
drivers/scsi/myrb.c | 8 ++++----
drivers/scsi/myrs.c | 14 +++++++-------
2 files changed, 11 insertions(+), 11 deletions(-)
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230828-scsi_fortify-9f8d279bf9aa
Best regards,
--
Nick Desaulniers <ndesaulniers@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] scsi: myrb: fix -Wfortify-source
2023-08-28 22:25 [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
@ 2023-08-28 22:25 ` Nick Desaulniers
2023-08-28 22:25 ` [PATCH 2/2] scsi: myrs: " Nick Desaulniers
2023-08-28 23:41 ` [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
2 siblings, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2023-08-28 22:25 UTC (permalink / raw)
To: Hannes Reinecke, James E.J. Bottomley, Martin K. Petersen,
Nathan Chancellor
Cc: Tom Rix, linux-scsi, linux-kernel, llvm, Nick Desaulniers
clang-18 has improved its support for detecting operations that will
truncate values at runtime via -Wfortify-source.
Fixes the warning:
drivers/scsi/myrb.c:1906:10: warning: 'snprintf' will always be
truncated; specified size is 32, but format string expands to at least
34 [-Wfortify-source]
In particular, the string literal "physical device - not rebuilding\n"
is indeed 34B by my count.
When we have a string literal that does not contain any format flags,
rather than use snprintf (sometimes with a size that's too small), let's
use sprintf.
This is pattern is cleaned up throughout the file.
Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1923
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
drivers/scsi/myrb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/myrb.c b/drivers/scsi/myrb.c
index ca2e932dd9b7..c2bdff36a6f1 100644
--- a/drivers/scsi/myrb.c
+++ b/drivers/scsi/myrb.c
@@ -1767,7 +1767,7 @@ static ssize_t raid_state_show(struct device *dev,
int ret;
if (!sdev->hostdata)
- return snprintf(buf, 16, "Unknown\n");
+ return sprintf(buf, "Unknown\n");
if (sdev->channel == myrb_logical_channel(sdev->host)) {
struct myrb_ldev_info *ldev_info = sdev->hostdata;
@@ -1890,7 +1890,7 @@ static ssize_t raid_level_show(struct device *dev,
ldev_info->state);
return snprintf(buf, 32, "%s\n", name);
}
- return snprintf(buf, 32, "Physical Drive\n");
+ return sprintf(buf, "Physical Drive\n");
}
static DEVICE_ATTR_RO(raid_level);
@@ -1903,13 +1903,13 @@ static ssize_t rebuild_show(struct device *dev,
unsigned char status;
if (sdev->channel < myrb_logical_channel(sdev->host))
- return snprintf(buf, 32, "physical device - not rebuilding\n");
+ return sprintf(buf, "physical device - not rebuilding\n");
status = myrb_get_rbld_progress(cb, &rbld_buf);
if (rbld_buf.ldev_num != sdev->id ||
status != MYRB_STATUS_SUCCESS)
- return snprintf(buf, 32, "not rebuilding\n");
+ return sprintf(buf, "not rebuilding\n");
return snprintf(buf, 32, "rebuilding block %u of %u\n",
rbld_buf.ldev_size - rbld_buf.blocks_left,
--
2.42.0.rc2.253.gd59a3bf2b4-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] scsi: myrs: fix -Wfortify-source
2023-08-28 22:25 [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
2023-08-28 22:25 ` [PATCH 1/2] scsi: myrb: fix -Wfortify-source Nick Desaulniers
@ 2023-08-28 22:25 ` Nick Desaulniers
2023-08-28 23:41 ` [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
2 siblings, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2023-08-28 22:25 UTC (permalink / raw)
To: Hannes Reinecke, James E.J. Bottomley, Martin K. Petersen,
Nathan Chancellor
Cc: Tom Rix, linux-scsi, linux-kernel, llvm, Nick Desaulniers
clang-18 has improved its support for detecting operations that will
truncate values at runtime via -Wfortify-source.
Fixes the warning:
drivers/scsi/myrs.c:1089:10: warning: 'snprintf' will always be
truncated; specified size is 32, but format string expands to at least
34 [-Wfortify-source]
In particular, the string literal "physical device - not rebuilding\n"
is indeed 34B by my count.
When we have a string literal that does not contain any format flags,
rather than use snprintf (sometimes with a size that's too small), let's
use sprintf.
This is pattern is cleaned up throughout the file.
Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1923
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
drivers/scsi/myrs.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index a1eec65a9713..729f08379bd0 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -939,7 +939,7 @@ static ssize_t raid_state_show(struct device *dev,
int ret;
if (!sdev->hostdata)
- return snprintf(buf, 16, "Unknown\n");
+ return sprintf(buf, "Unknown\n");
if (sdev->channel >= cs->ctlr_info->physchan_present) {
struct myrs_ldev_info *ldev_info = sdev->hostdata;
@@ -1058,7 +1058,7 @@ static ssize_t raid_level_show(struct device *dev,
const char *name = NULL;
if (!sdev->hostdata)
- return snprintf(buf, 16, "Unknown\n");
+ return sprintf(buf, "Unknown\n");
if (sdev->channel >= cs->ctlr_info->physchan_present) {
struct myrs_ldev_info *ldev_info;
@@ -1086,7 +1086,7 @@ static ssize_t rebuild_show(struct device *dev,
unsigned char status;
if (sdev->channel < cs->ctlr_info->physchan_present)
- return snprintf(buf, 32, "physical device - not rebuilding\n");
+ return sprintf(buf, "physical device - not rebuilding\n");
ldev_info = sdev->hostdata;
ldev_num = ldev_info->ldev_num;
@@ -1102,7 +1102,7 @@ static ssize_t rebuild_show(struct device *dev,
(size_t)ldev_info->rbld_lba,
(size_t)ldev_info->cfg_devsize);
} else
- return snprintf(buf, 32, "not rebuilding\n");
+ return sprintf(buf, "not rebuilding\n");
}
static ssize_t rebuild_store(struct device *dev,
@@ -1190,7 +1190,7 @@ static ssize_t consistency_check_show(struct device *dev,
unsigned short ldev_num;
if (sdev->channel < cs->ctlr_info->physchan_present)
- return snprintf(buf, 32, "physical device - not checking\n");
+ return sprintf(buf, "physical device - not checking\n");
ldev_info = sdev->hostdata;
if (!ldev_info)
@@ -1202,7 +1202,7 @@ static ssize_t consistency_check_show(struct device *dev,
(size_t)ldev_info->cc_lba,
(size_t)ldev_info->cfg_devsize);
else
- return snprintf(buf, 32, "not checking\n");
+ return sprintf(buf, "not checking\n");
}
static ssize_t consistency_check_store(struct device *dev,
@@ -1376,7 +1376,7 @@ static ssize_t processor_show(struct device *dev,
info->cpu[1].cpu_name,
second_processor, info->cpu[1].cpu_count);
else
- ret = snprintf(buf, 64, "1: absent\n2: absent\n");
+ ret = sprintf(buf, "1: absent\n2: absent\n");
return ret;
}
--
2.42.0.rc2.253.gd59a3bf2b4-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source
2023-08-28 22:25 [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
2023-08-28 22:25 ` [PATCH 1/2] scsi: myrb: fix -Wfortify-source Nick Desaulniers
2023-08-28 22:25 ` [PATCH 2/2] scsi: myrs: " Nick Desaulniers
@ 2023-08-28 23:41 ` Nick Desaulniers
2023-08-29 16:33 ` Nick Desaulniers
2 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2023-08-28 23:41 UTC (permalink / raw)
To: Hannes Reinecke, James E.J. Bottomley, Martin K. Petersen,
Nathan Chancellor
Cc: Tom Rix, linux-scsi, linux-kernel, llvm, Ard Biesheuvel
On Mon, Aug 28, 2023 at 3:25 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> clang-18 has improved its support for detecting operations that will
> truncate values at runtime via -wfortify-source resulting in two new
^ -Wfortify-source
> warnings (or errors with CONFIG_WERROR=y):
>
> drivers/scsi/myrb.c:1906:10: warning: 'snprintf' will always be
> truncated; specified size is 32, but format string expands to at least
> 34 [-Wfortify-source]
>
> drivers/scsi/myrs.c:1089:10: warning: 'snprintf' will always be
> truncated; specified size is 32, but format string expands to at least
> 34 [-Wfortify-source]
>
> When we have a string literal that does not contain any format flags,
> rather than use snprintf (sometimes with a size that's too small), let's
> use sprintf.
Even better, Ard points out this could be strcpy (or one of the
variants). Will send a v2 tomorrow.
>
> This is pattern is cleaned up throughout two files.
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Nick Desaulniers (2):
> scsi: myrb: fix -Wfortify-source
> scsi: myrs: fix -Wfortify-source
>
> drivers/scsi/myrb.c | 8 ++++----
> drivers/scsi/myrs.c | 14 +++++++-------
> 2 files changed, 11 insertions(+), 11 deletions(-)
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230828-scsi_fortify-9f8d279bf9aa
>
> Best regards,
> --
> Nick Desaulniers <ndesaulniers@google.com>
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source
2023-08-28 23:41 ` [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
@ 2023-08-29 16:33 ` Nick Desaulniers
2023-08-29 18:48 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2023-08-29 16:33 UTC (permalink / raw)
To: Kees Cook
Cc: Tom Rix, linux-scsi, linux-kernel, llvm, Ard Biesheuvel,
Nathan Chancellor, Martin K. Petersen, James E.J. Bottomley,
Hannes Reinecke
On Mon, Aug 28, 2023 at 4:41 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Aug 28, 2023 at 3:25 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > clang-18 has improved its support for detecting operations that will
> > truncate values at runtime via -wfortify-source resulting in two new
>
> ^ -Wfortify-source
>
> > warnings (or errors with CONFIG_WERROR=y):
> >
> > drivers/scsi/myrb.c:1906:10: warning: 'snprintf' will always be
> > truncated; specified size is 32, but format string expands to at least
> > 34 [-Wfortify-source]
> >
> > drivers/scsi/myrs.c:1089:10: warning: 'snprintf' will always be
> > truncated; specified size is 32, but format string expands to at least
> > 34 [-Wfortify-source]
> >
> > When we have a string literal that does not contain any format flags,
> > rather than use snprintf (sometimes with a size that's too small), let's
> > use sprintf.
>
> Even better, Ard points out this could be strcpy (or one of the
> variants). Will send a v2 tomorrow.
Oh strcpy doesn't return the number of bytes copied, which is what the
users here need.
(The size of dst is also unknown).
Any thoughts on?
/* strcpy but return the length of src and requires a literal. */
#define strcpy_literal(dst, src) ({ \
strcpy(dst, src ""); \
__builtin_strlen(src); \
})
Uses a trick I learned from Abseil for ensuring that a parameter must
be a string literal. The C preprocessor will concatenate those
strings, or parsing will fail.
Then these drivers can do:
return strcpy_literal(buf, "physical device - not checking\n");
If that's not blood curdling, any thoughts on where best to place this
to hide it from others? Maybe just define it in both .c files?
>
> >
> > This is pattern is cleaned up throughout two files.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > Nick Desaulniers (2):
> > scsi: myrb: fix -Wfortify-source
> > scsi: myrs: fix -Wfortify-source
> >
> > drivers/scsi/myrb.c | 8 ++++----
> > drivers/scsi/myrs.c | 14 +++++++-------
> > 2 files changed, 11 insertions(+), 11 deletions(-)
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id: 20230828-scsi_fortify-9f8d279bf9aa
> >
> > Best regards,
> > --
> > Nick Desaulniers <ndesaulniers@google.com>
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source
2023-08-29 16:33 ` Nick Desaulniers
@ 2023-08-29 18:48 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-08-29 18:48 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Tom Rix, linux-scsi, linux-kernel, llvm, Ard Biesheuvel,
Nathan Chancellor, Martin K. Petersen, James E.J. Bottomley,
Hannes Reinecke
On Tue, Aug 29, 2023 at 09:33:55AM -0700, Nick Desaulniers wrote:
> On Mon, Aug 28, 2023 at 4:41 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Mon, Aug 28, 2023 at 3:25 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > clang-18 has improved its support for detecting operations that will
> > > truncate values at runtime via -wfortify-source resulting in two new
> >
> > ^ -Wfortify-source
> >
> > > warnings (or errors with CONFIG_WERROR=y):
> > >
> > > drivers/scsi/myrb.c:1906:10: warning: 'snprintf' will always be
> > > truncated; specified size is 32, but format string expands to at least
> > > 34 [-Wfortify-source]
> > >
> > > drivers/scsi/myrs.c:1089:10: warning: 'snprintf' will always be
> > > truncated; specified size is 32, but format string expands to at least
> > > 34 [-Wfortify-source]
These should just use sysfs_emit() instead. Then all the bounds checking
against the PAGE_SIZE buffer gets done correctly, etc.
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-29 18:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-28 22:25 [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
2023-08-28 22:25 ` [PATCH 1/2] scsi: myrb: fix -Wfortify-source Nick Desaulniers
2023-08-28 22:25 ` [PATCH 2/2] scsi: myrs: " Nick Desaulniers
2023-08-28 23:41 ` [PATCH 0/2] scsi: fix 2 cases of -Wfortify-source Nick Desaulniers
2023-08-29 16:33 ` Nick Desaulniers
2023-08-29 18:48 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).