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

[Benjamin Marzinski]

On Tue, Nov 30, 2021 at 09:14:10PM +0100, Martin Wilck wrote:
> On Tue, 2021-11-30 at 11:30 -0600, Benjamin Marzinski wrote:
> > 
> > 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.
> 
> That's what my code does (embarrassingly, I didn't realize that when I
> wrote my previous reply). 
> 
> In shortened pseudo code:
> 
> 	int prio = -1;
> 	int err = -ENODATA;
> 
> 	d = first_designator();
> 	while (d <= in + in_len - 4) {
> 		bool invalid = false;
> 		int new_prio = -1;
> 
>                 if (designator_is_invalid(d))
>                      invalid = true;
>                 else if (lun_association(d))
>                      new_prio = prio(d);
> 
> 	next_designator:
> 		if (invalid) {
> 			err = -EINVAL;
> 			break;  /** see below **/
> 		}
> 		if (new_prio > prio) {
> 			vpd = d;
> 			prio = new_prio;
> 		}
> 		d = next_designator(d);
> 	}
> 
>         /* if we did find a valid designator, prio will be > 0, and we
>            will not error out */
> 	if (prio <= 0)
> 		return err;
> 
>         convert_designator_to_wwid(d);
> 
> If you think we should use a different strategy, please explain.
> We *could* try to go on even after encountering broken designators,
> assuming the length field is correct if it's within the given limits,
> even if the rest is bogus. So baiscally instead of the break statement
> above, we'd continue the loop.
> 
> Would you prefer that?

Again looking at sg_inq, it will loop through all the descriptors,
trusting the length field, until it gets to the whole data length. It
prints a warning if it doesn't end up at exactly the data length, but
still exports all the IDs it finds.  If an individual descriptor doesn't
make sense, it gets skipped. That would be my preference.

-Ben

> Regards,
> Martin