[dm-devel] [PATCH 05/21] libmultipath (coverity): improve input checking in parse_vpd_pg83

[Benjamin Marzinski]

On Tue, Nov 30, 2021 at 12:21:44PM +0100, Martin Wilck wrote:
> On Mon, 2021-11-29 at 19:17 -0600, Benjamin Marzinski wrote:
> > On Fri, Nov 19, 2021 at 12:13:22AM +0100, mwilck at suse.com wrote:
> > > From: Martin Wilck <mwilck at suse.com>
> > > 
> > > Check offsets and other obvious errors in the VPD83 data.
> > > 
> > > The original reason to do this was to fix "tained scalar"
> > > warnings from coverity. But this doesn't suffice for coverity
> > > without using a constant boundary (WWID_SIZE) for "len" in
> > > parse_vpd_pg80() and for "vpd_len" in parse_vpd_pg83(), even
> > > though the computed boundaries are tighter than the constant
> > > ones.
> > > 
> > 
> > This looks fine, but I do wonder if we are being too strict.  I'm not
> > sure we should be failing, if sg_inq wouldn't fail.  The goal of the
> > fallback code is to get the WWID the udev would have gotten, and
> > being
> > lenient with scsi devices that don't quite follow the standard is
> > usually the best policy. Thoughts?
> 
> If we encounter a broken VPD entry, IMO we can't continue reading
> further entries, because if the entry is broken, we can't trust the
> length value which is part of the entry. We may be reading total junk
> if we follow it.
> 
> We might simply discard the broken entry, stop iterating over the
> designators, see if we got a usable designator up to that point (i.e.,
> previously), and if yes, use that entry, printing a big fat warning. 
> That means we'd ultimately fail only if there was no usable designator
> before the broken one.
> 
> Would you prefer that strategy?

If you export scsi IDs from sg_inq, it will print them out until it
hits an error, so I would prefer if we used the best designator we got
before we hit the error, to match it.

-Ben

> 
> Martin