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

[Martin Wilck]

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.

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