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

[Benjamin Marzinski]

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:
> > On Mon, 2021-02-01 at 22:50 +0800, lixiaokeng wrote:
> > > 
> > > > > 
> > > > > cli_add_path
> > > > >    ->ev_add_path
> > > > >       ->add_map_with_path
> > > > >          ->adopt_paths
> > > > >             ->pathinfo
> > > > >                ->filter_property
> > > > >                ->return PATHINFO_SKIPPED,
> > > > >             ->pp->mpp is NULL and not be set
> > > > >             ->return 0
> > > > 
> > > > This returns 0, but add_map_with_path() has this code to check
> > > > whether
> > > > the path passed to it was actually added to the new map:
> > > > 
> > > >         if (adopt_paths(vecs->pathvec, mpp) ||
> > > >             find_slot(vecs->pathvec, pp) == -1)
> > > >                 goto out;  -> return NULL
> > > > 
> > > > So ev_add_path() should have seen a NULL return from
> > > > add_map_with_path(), should not have set start_waiter, and failed. 
> > > > 
> > > 
> > > I'm sorry for a big mistake in my stack. As the code is optimized,
> > > pathinfo
> > > return PATHINFO_SKIPPED after finish filter_property when I use gdb.
> > > It
> > > happens acctualy in:
> > > 2141                    if (pp->bus == SYSFS_BUS_SCSI &&
> > > 2142                        pp->sg_id.proto_id == SCSI_PROTOCOL_USB
> > > &&
> > > 2143                        !conf->allow_usb_devices) {
> > > 2144                            condlog(3, "%s: skip USB device %s",
> > > pp->dev,
> > > 2145                                    pp->tgt_node_name);
> > > 2146                            return PATHINFO_SKIPPED;
> > > 2147                    }
> > > 2148            }
> > > 
> > > Stack:
> > > cli_add_path
> > >    ->ev_add_path
> > >       ->add_map_with_path
> > >          ->adopt_paths
> > >             ->pathinfo
> > >                ->pp->bus == SYSFS_BUS_SCSI
> > >                ->return PATHINFO_SKIPPED,
> > >             ->pp->mpp is NULL and not be set
> > >             ->return 0
> > >       ->mpath_pr_event_handle
> > >          ->get_be64 //pp->mpp is dereference
> > > 
> > > If you think my patch is ok, I will resend it.
> > 
> > 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.

-Ben

> 
> However, it will fail in the pathinfo call from adopt_paths, so the path
> won't be added to the multipath device.  This means adopt paths doesn't
> actually adopt any paths potentially, but that in itself doesn't cause
> it to fail. This check
> 
>         if (adopt_paths(vecs->pathvec, mpp) ||
>             find_slot(vecs->pathvec, pp) == -1)
>                 goto out;
> 
> passes, since we only check if the path is on the pathvec, not part of
> the multipath device, and since filter_property let the path past the
> first time, it is. So add_map_with_path() will create a multipath
> device, but the path won't be added to it, and pp->mpp == NULL.
> 
> So, add_map_with_path() should probably check that we actually created a
> map that included the path that got added. But more importantly,
> filter_property shouldn't return different results the when it's called
> the first time.  That would have avoid the entire situation.
> 
> -Ben
> 
> 
> > Your patch would only be effective if it was possible that
> > add_map_with_path(vecs, pp, 1) returned an mpp != NULL, and at the same
> > time pp->mpp was NULL; I still don't understand how that can come to
> > pass.
> > 
> > Have you tried my patch?
> > 
> > Regards,
> > Martin
> > 
> >