[0/2] scsi: fix 2 cases of -Wfortify-source

Message

Nick Desaulniers Aug. 28, 2023, 10:25 p.m. UTC

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 <[e-mail]>
---
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,

Comments

Nick Desaulniers Aug. 28, 2023, 11:41 p.m. UTC | #1

On Mon, Aug 28, 2023 at 3:25 PM Nick Desaulniers
<[e-mail]> 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 <[e-mail]>
> ---
> 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 <[e-mail]>
>

Nick Desaulniers Aug. 29, 2023, 4:33 p.m. UTC | #2

On Mon, Aug 28, 2023 at 4:41 PM Nick Desaulniers
<[e-mail]> wrote:
>
> On Mon, Aug 28, 2023 at 3:25 PM Nick Desaulniers
> <[e-mail]> 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 <[e-mail]>
> > ---
> > 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 <[e-mail]>
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

Kees Cook Aug. 29, 2023, 6:48 p.m. UTC | #3

On Tue, Aug 29, 2023 at 09:33:55AM -0700, Nick Desaulniers wrote:
> On Mon, Aug 28, 2023 at 4:41 PM Nick Desaulniers
> <[e-mail]> wrote:
> >
> > On Mon, Aug 28, 2023 at 3:25 PM Nick Desaulniers
> > <[e-mail]> 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.