[dm-devel] libmultipath: fix NULL dereference in get_be64

[Martin Wilck]

On Tue, 2021-02-02 at 01:18 -0600, Benjamin Marzinski wrote:
> On Mon, Feb 01, 2021 at 11:26:36PM -0600, Benjamin Marzinski wrote:
> > On Mon, Feb 01, 2021 at 04:12:34PM +0100, Martin Wilck wrote:
> > > 
> > > The same argument I made above still holds. "pp" wouldn't have
> > > been
> > > added to mpp, and add_map_with_path() would fail and return NULL.
> > > Also, if pathinfo() returns PATHINFO_SKIPPED for this device,
> > > how comes that cli_add_path() called ev_add_path() for it? It
> > > should
> > > have returned "blacklisted" instead.
> > 
> > So, I think the main issue here is that filter_property appears to
> > be
> > broken.  It only filters if uid_attribute is set, but that will
> > never be
> > set the first time it's called in pathinfo.  This means that it
> > will
> > pass in the pathinfo call in cli_add_path, and the path will get
> > stored
> > in the pathvec.
> 
> Just to be a little more clear here, filter_property() will only
> return
> MATCH_PROPERTY_BLIST_MISSING for missing udev properties, if the
> uid_attribute is set and seen. We should probably make sure to set
> uid_attribute before calling it.

I've just spent a few hours trying to figure this out. Unfortunately,
it isn't easy. pp->uid_attribute may be set from the hwtable, which
means that vendor/product must be known, which in turn means that
uid_attribute can't be set correctly before sysfs_pathinfo() is run.

Thus, in order to be consistent, we need to move the filter_property()
quite a bit further down in pathinfo(), after the call to
sysfs_pathinfo(). That can be done, I already have a (basically)
working code here (most of the work was fixing the unit tests).

*However*, that has a side effect. As you said correctly above, 
pp->uid_attribute currently is *never* set the first time the call
chain pathinfo()->filter_property() is run. After the proposed change,
it would *always* be set in this call chain, possibly leading to more
cases where pathinfo returns PATHINFO_SKIPPED.

AFAICS this would matter e.g. for "multipath -w". When we remove a
WWID, we must be sure get_refwwid() fills in the wwid, which won't
happen if PATHINFO_SKIPPED is returned. (This would only affect paths
that filter_property() would skip, so it's just a corner case, yet it
might confuse people in certain situations).

In order not to break such use cases, we need to make the
filter_property() test in pathinfo() dependent on DI_BLACKLIST. That
would make a lot of sense, but it'd cause another semantic change.
d51da42 ("libmultipath: move filter_property|devnode() from
path_discover() into pathinfo()") had deliberately not made this
change.

Well, I'll submit a patch set, in order to make you see how this would
end up looking. But I wouldn't propose to queue that up for mainline
just yet. I strongly hope that the fix for add_map_with_path() alone
will fix lixiaokeng's issue.

Cheers,
Martin