Re: [PATCH v3 2/6] scsi: Protect INQUIRY sysfs attributes with mutex

[Brian Bunker <brian@purestorage.com>]

Bart, Hannes,

Before I respin, I'd like to get your alignment on the API design.

Hannes, in your review of the earlier single-patch version of this
work last year, you wrote:

> +	sdev->vendor = (char *)(sdev->inquiry + 8);
> +	sdev->model = (char *)(sdev->inquiry + 16);
> +	sdev->rev = (char *)(sdev->inquiry + 32);
>
> I really hate these.
> Can't we replace them with accessor functions and drop the pointers?

That's the direction I want to take. Bart's feedback on v3 2/6 then
pushed back specifically on accessor functions that hand out pointers
to character data which is not '\0' terminated, and suggested that
sdev_show_##field() could take an offset and length instead.

Those two pieces of feedback converge: drop the cached pointers, but
don't replace them with anything that exposes a pointer to
non-NUL-terminated bytes. Before I respin around that, I want to make
sure I have the scope of Bart's objection right.

To help frame it, I audited every call site in the tree that touches
sdev->vendor, sdev->model, or sdev->rev - both the direct field name
and the alias forms callers actually use (cp->device->vendor,
cd->device->vendor, scsidp->vendor, ch->dt[elem]->vendor, SDp->vendor,
STp->device->..., etc.). For each hit I checked two failure modes:

  - bare "%s" in a format string (the qla2xxx pattern); and
  - NUL-assuming string functions (strlen, strcpy, strcat, strdup,
    strstr, strchr, strrchr, strcmp, strcasecmp, strscpy, strlcpy).

Result, across all three fields:

  Site                                              Field(s)   NUL-term?
  --------------------------------------------------  ---------  ----
  drivers/hwmon/drivetemp.c            (strncmp)       model       no
  drivers/scsi/aacraid/linit.c         (strncmp)       vendor      no
  drivers/scsi/advansys.c              (strncmp)       vendor      no
  drivers/scsi/scsi_dh.c               (strncmp)       v, m        no
  drivers/scsi/scsi_proc.c             (bounded loop)  v, m, r     no
  drivers/scsi/scsi_scan.c             (strncmp/%.Ns)  v, m, r     no
  drivers/scsi/sg.c                    (%8.8s)         v, m, r     no
  drivers/scsi/sr_vendor.c             (strncmp)       v, m        no
  drivers/scsi/scsi_sysfs.c            (%.Ns)          v, m, r     no
  drivers/scsi/st.c                    (strncmp)       v, m, r     no
  drivers/scsi/ch.c                    (%8.8s)         v, m, r     no
  drivers/scsi/storvsc_drv.c           (strncmp)       vendor      no
  drivers/target/target_core_pscsi.c   (snprintf %.Ns) v, m, r     no
  drivers/scsi/qla2xxx/qla_isr.c:3664  ("%s")          vendor      YES

Of 18 distinct call sites, 17 are bounded by either an explicit
length or a precision specifier. Exactly one - qla2xxx - passes
cp->device->vendor to a "%s" format, which walks past the 8-byte
vendor field into the model field until it finds a NUL byte. That's
a real over-read today.

I also checked every strlen/strcpy/strcat/etc. call site in the tree
and confirmed none of them runs on sdev->vendor / sdev->model /
sdev->rev directly. The two scsi_dh.c and st.c sites that look like
they might (strncmp(sdev->vendor, blacklist->vendor,
strlen(blacklist->vendor))) actually call strlen on the blacklist
entry, which is a NUL-terminated literal - the strncmp is bounded by
that length on the sdev side. Safe.

So there are no NUL-termination misuses on model or rev anywhere in
the tree, and exactly one on vendor (qla2xxx).

That means the work has two motivations that are worth separating:

  (a) qla2xxx is a bug today and worth fixing on its own with a
      Fixes: tag, independent of any larger refactoring.

  (b) The remaining 17 sites work today but only because each author
      knew the field is space-padded with no terminator. The
      conversion series doesn't fix bugs at those sites; it removes
      the opportunity for the next person to write "%s" somewhere and
      reintroduce a qla2xxx-class bug, and it's preparatory for
      INQUIRY refresh on rescan (where the buffer can be reallocated
      and a raw pointer into it becomes a UAF risk).

For (b), I had been heading toward an API built around copy-out and
match helpers - i.e. nothing in the public API hands back a pointer
to non-NUL-terminated bytes:

  size_t scsi_device_vendor(struct scsi_device *sdev,
                            char *buf, size_t len);
  size_t scsi_device_model(struct scsi_device *sdev,
                           char *buf, size_t len);
  size_t scsi_device_rev(struct scsi_device *sdev,
                         char *buf, size_t len);

  bool   scsi_device_vendor_match(struct scsi_device *sdev,
                                  const char *prefix);
  bool   scsi_device_model_match(struct scsi_device *sdev,
                                 const char *prefix);
  bool   scsi_device_rev_match(struct scsi_device *sdev,
                               const char *prefix);

The copy helpers take a caller buffer, copy the field, and
NUL-terminate. The match helpers take a NUL-terminated prefix string
from the caller and do a bounded comparison against the
(non-terminated) field bytes. The earlier scsi_inq_vendor() /
scsi_inq_product() / scsi_inq_revision() helpers that returned a
const char * into the inquiry buffer have been dropped.

A few questions before I respin:

  1. Bart, does the copy-out / match API above address the "no
     accessors for non-NUL-terminated data" concern, or is the
     objection broader?

  2. Hannes, would you prefer the conversion patches and the
     removal of sdev->vendor / sdev->model / sdev->rev to land as
     one series, or split into two - conversions first, removal as a
     follow-up once the conversions have settled?

I'd rather get your input on the API before sending another
12+ patch respin.

Thanks,
Brian