LinuxLists.cc - [PATCH 00/10] scsi: Replace {v}snprintf() variants with safer alternatives

2024-02-08 08:47:28

Subject: [PATCH 00/10] scsi: Replace {v}snprintf() variants with safer alternatives

Note: We're also taking the time to obay our new .editorconfig overlord!

For a far better description of the problem than I could author, see
Jon's write-up on LWN [1] and/or Alex's on the Kernel Self Protection
Project [1].

[0] https://lwn.net/Articles/69419/
[1] https://github.com/KSPP/linux/issues/105

Lee Jones (10):
scsi: 3w-xxxx: Trivial: Remove trailing whitespace
scsi: 53c700: Trivial: Remove trailing whitespace
scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant
scsi: aacraid: linit: Remove snprintf() from sysfs call-backs and
replace with sysfs_emit()
scsi: aacraid: linit: Replace snprintf() with the safer scnprintf()
variant
scsi: aha1542: Replace snprintf() with the safer scnprintf() variant
scsi: aic7xxx: aicasm: Trivial: Remove trailing whitespace
scsi: aic7xxx: aicasm: Replace snprintf() with the safer scnprintf()
variant
scsi: aic94xx: Remove snprintf() from sysfs call-backs and replace
with sysfs_emit()
scsi: arcmsr: Remove snprintf() from sysfs call-backs and replace with
sysfs_emit()

drivers/scsi/3w-xxxx.c | 2 +-
drivers/scsi/53c700.c | 102 +++++++++++++--------------
drivers/scsi/NCR5380.c | 16 ++---
drivers/scsi/aacraid/linit.c | 40 +++++------
drivers/scsi/aha1542.c | 2 +-
drivers/scsi/aic7xxx/aicasm/aicasm.c | 16 ++---
drivers/scsi/aic94xx/aic94xx_init.c | 11 ++-
drivers/scsi/arcmsr/arcmsr_attr.c | 40 +++--------
8 files changed, 101 insertions(+), 128 deletions(-)

Cc: Adam Radford <[email protected]>
Cc: Adaptec OEM Raid Solutions <[email protected]>
Cc: Andre Hedrick <[email protected]>
Cc: de Melo <[email protected]>
Cc: [email protected]
Cc: Finn Thain <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: Joel Jacobson <[email protected]>
Cc: John Garry <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: [email protected]
Cc: Luben Tuikov <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: Michael Schmitz <[email protected]>
Cc: "PMC-Sierra, Inc" <[email protected]>
Cc: Richard Hirst <[email protected]>
Cc: [email protected]
Cc: Tnx to <[email protected]>
--
2.43.0.594.gd9cf4e227d-goog

2024-02-08 08:47:46

by Lee Jones

[permalink] [raw]

2024-02-08 08:50:10

by Lee Jones

[permalink] [raw]

Subject: [PATCH 08/10] scsi: aic7xxx: aicasm: Replace snprintf() with the safer scnprintf() variant

2024-02-08 08:50:20

by Lee Jones

[permalink] [raw]

Subject: [PATCH 10/10] scsi: arcmsr: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

Since snprintf() has the documented, but still rather strange trait of
returning the length of the data that *would have been* written to the
array if space were available, rather than the arguably more useful
length of data *actually* written, it is usually considered wise to use
something else instead in order to avoid confusion.

In the case of sysfs call-backs, new wrappers exist that do just that.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <[email protected]>
---
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/scsi/arcmsr/arcmsr_attr.c | 40 ++++++++-----------------------
1 file changed, 10 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
index baeb5e7956902..c430c835503be 100644
--- a/drivers/scsi/arcmsr/arcmsr_attr.c
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c
@@ -258,9 +258,7 @@ static ssize_t
arcmsr_attr_host_driver_version(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return snprintf(buf, PAGE_SIZE,
- "%s\n",
- ARCMSR_DRIVER_VERSION);
+ return sysfs_emit(buf, "%s\n", ARCMSR_DRIVER_VERSION);
}

static ssize_t
@@ -270,9 +268,7 @@ arcmsr_attr_host_driver_posted_cmd(struct device *dev,
struct Scsi_Host *host = class_to_shost(dev);
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;
- return snprintf(buf, PAGE_SIZE,
- "%4d\n",
- atomic_read(&acb->ccboutstandingcount));
+ return sysfs_emit(buf, "%4d\n", atomic_read(&acb->ccboutstandingcount));
}

static ssize_t
@@ -282,9 +278,7 @@ arcmsr_attr_host_driver_reset(struct device *dev,
struct Scsi_Host *host = class_to_shost(dev);
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;
- return snprintf(buf, PAGE_SIZE,
- "%4d\n",
- acb->num_resets);
+ return sysfs_emit(buf, "%4d\n", acb->num_resets);
}

static ssize_t
@@ -294,9 +288,7 @@ arcmsr_attr_host_driver_abort(struct device *dev,
struct Scsi_Host *host = class_to_shost(dev);
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;
- return snprintf(buf, PAGE_SIZE,
- "%4d\n",
- acb->num_aborts);
+ return sysfs_emit(buf, "%4d\n", acb->num_aborts);
}

static ssize_t
@@ -306,9 +298,7 @@ arcmsr_attr_host_fw_model(struct device *dev, struct device_attribute *attr,
struct Scsi_Host *host = class_to_shost(dev);
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;
- return snprintf(buf, PAGE_SIZE,
- "%s\n",
- acb->firm_model);
+ return sysfs_emit(buf, "%s\n", acb->firm_model);
}

static ssize_t
@@ -319,9 +309,7 @@ arcmsr_attr_host_fw_version(struct device *dev,
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;

- return snprintf(buf, PAGE_SIZE,
- "%s\n",
- acb->firm_version);
+ return sysfs_emit(buf, "%s\n", acb->firm_version);
}

static ssize_t
@@ -332,9 +320,7 @@ arcmsr_attr_host_fw_request_len(struct device *dev,
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;

- return snprintf(buf, PAGE_SIZE,
- "%4d\n",
- acb->firm_request_len);
+ return sysfs_emit(buf, "%4d\n", acb->firm_request_len);
}

static ssize_t
@@ -345,9 +331,7 @@ arcmsr_attr_host_fw_numbers_queue(struct device *dev,
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;

- return snprintf(buf, PAGE_SIZE,
- "%4d\n",
- acb->firm_numbers_queue);
+ return sysfs_emit(buf, "%4d\n", acb->firm_numbers_queue);
}

static ssize_t
@@ -358,9 +342,7 @@ arcmsr_attr_host_fw_sdram_size(struct device *dev,
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;

- return snprintf(buf, PAGE_SIZE,
- "%4d\n",
- acb->firm_sdram_size);
+ return sysfs_emit(buf, "%4d\n", acb->firm_sdram_size);
}

static ssize_t
@@ -371,9 +353,7 @@ arcmsr_attr_host_fw_hd_channels(struct device *dev,
struct AdapterControlBlock *acb =
(struct AdapterControlBlock *) host->hostdata;

- return snprintf(buf, PAGE_SIZE,
- "%4d\n",
- acb->firm_hd_channels);
+ return sysfs_emit(buf, "%4d\n", acb->firm_hd_channels);
}

static DEVICE_ATTR(host_driver_version, S_IRUGO, arcmsr_attr_host_driver_version, NULL);
--
2.43.0.594.gd9cf4e227d-goog

2024-02-08 08:50:24

by Lee Jones

[permalink] [raw]

On Mon, 19 Feb 2024, Kees Cook wrote:

> On Mon, Feb 19, 2024 at 03:23:12PM +0000, Lee Jones wrote:
> > Adding this to checkpatch is a good idea.
>
> Yeah, please do. You can look at the "strncpy -> strscpy" check that is
> already in there for an example.
>
> >
> > What if we also take Kees's suggestion and hit all of these found in
> > SCSI in one patch to keep the churn down to a minimum?
>
> We don't have to focus on SCSI even. At the end of the next -rc1, I can

When I've conducted similar work before, I've taken it subsystem by
subsystem. However, if you're happy to co-ordinate with the big penguin
et al. and get them all with a treewide patch, please go for it.

> send a tree-wide patch (from Coccinelle) that'll convert all snprintf()
> uses that don't check a return value into scnprintf(). For example,
> this seems to do the trick:
>
> @scnprintf depends on !(file in "tools") && !(file in "samples")@
> @@
>
> -snprintf
> +scnprintf
> (...);
>
>
> Results in:
>
> 2252 files changed, 4795 insertions(+), 4795 deletions(-)

Super!

--
Lee Jones [李琼斯]

2024-02-20 08:28:39

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 03/10] scsi: NCR5380: Replace snprintf() with the safer scnprintf() variant

On Mon, 19 Feb 2024, James Bottomley wrote:

> On Mon, 2024-02-19 at 15:23 +0000, Lee Jones wrote:
> > On Sat, 10 Feb 2024, James Bottomley wrote:
> >
> > > On Thu, 2024-02-08 at 10:29 +0000, Lee Jones wrote:
> > > > On Thu, 08 Feb 2024, Geert Uytterhoeven wrote:
> > > >
> > > > > Hi Lee,
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > On Thu, Feb 8, 2024 at 9:48 AM Lee Jones <[email protected]>
> > > > > wrote:
> > > > > > There is a general misunderstanding amongst engineers that
> > > > > > {v}snprintf() returns the length of the data *actually*
> > > > > > encoded into the destination array. However, as per the C99
> > > > > > standard {v}snprintf() really returns the length of the data
> > > > > > that *would have been* written if there were enough space for
> > > > > > it. This misunderstanding has led to buffer-overruns in the
> > > > > > past. It's generally considered safer to use the
> > > > > > {v}scnprintf() variants in their place (or even sprintf() in
> > > > > > simple cases). So let's do that.
> > > > >
> > > > > Confused... The return value is not used at all?
> > > >
> > > > Future proofing. The idea of the effort is to rid the use
> > > > entirely.
> > > >
> > > > - Usage is inside a sysfs handler passing PAGE_SIZE as the size
> > > >    - s/snprintf/sysfs_emit/
> > > > - Usage is inside a sysfs handler passing a bespoke value as the
> > > > size
> > > >    - s/snprintf/scnprintf/
> > > > - Return value used, but does *not* care about overflow
> > > >    - s/snprintf/scnprintf/
> > > > - Return value used, caller *does* care about overflow
> > > >    - s/snprintf/seq_buf/
> > > > - Return value not used
> > > >    - s/snprintf/scnprintf/
> > > >
> > > > This is the final case.
> > >
> > > To re-ask Geert's question: the last case can't ever lead to a bug
> > > orproblem, what value does churning the kernel to change it
> > > provide? As Finn said, if we want to deprecate it as a future
> > > pattern, put it in checkpatch.
> >
> > Adding this to checkpatch is a good idea.
> >
> > What if we also take Kees's suggestion and hit all of these found in
> > SCSI in one patch to keep the churn down to a minimum?
>
> That doesn't fix the churn problem because you're still changing the
> source. For ancient drivers, we keep the changes to a minimum to avoid
> introducing inadvertent bugs which aren't discovered until months
> later. If there's no actual bug in the driver, there's no reason to
> change the code.

Okay, no problem. Would you like me to drop these from the set and
resubmit or are you happy to cherry-pick the remainder?

--
Lee Jones [李琼斯]