[PATCH 0/4] Fix segfault on access to mpp->hwe

[PATCH 0/4] Fix segfault on access to mpp->hwe

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This is in response to Lixiaokeng's post
"master - libmultipath: fix use after free when iscsi logs in".

On top of Lixiaokeng's patch, I've added some log messages and
a fix for mpp->hwe handling in sync_paths().

The question remains how we handle maps without paths. I believe we're good
here, please review my assessment.

mpp->hwe is only accessed in propsel.c, via the mp_set_hwe() macro. Patch 3 of
my series adds an error message if this happens while mpp->hwe is NULL. IMO it
shouldn't happen because we don't check map properties for empty
maps. Normally this is done when a map is created, and we don't create maps
without paths.

The case where a map looses all paths during its normal lifetime and
can't be removed (e.g. because it's busy) is already covered by the
current code AFAICT. When a new path is re-added, we'll call adopt_paths
and verify_paths(), which will make sure that mpp->hwe is set again
to the pp->hwe member of the newly added path.

Reviews and comments welcome.

Regards
Martin

Martin Wilck (3):
  libmultipath: warn if freeing path that holds mpp->hwe
  libmultipath: warn about NULL value of mpp->hwe
  libmultipath: fix mpp->hwe handling in sync_paths()

lixiaokeng (1):
  master - libmultipath: fix use after free when iscsi logs in

 libmultipath/propsel.c     | 4 +++-
 libmultipath/structs_vec.c | 9 +++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH 1/4] master - libmultipath: fix use after free when iscsi logs in

[mwilck]

From: lixiaokeng <lixiaokeng@huawei.com>

When two iscsi ips log in and out alternately and  the following scripts run
at the same time,

#!/bin/bash
interval=5
while true
do
        iscsiadm -m node -p 9.41.147.171 &> /dev/null
        iscsiadm -m node -p 9.41.148.172 &> /dev/null
        iscsiadm -m session &> /dev/null
        rescan-scsi-bus.sh &> /dev/null
        multipath -v2 &> /dev/null
        multipath -ll &> /dev/null
        sleep $interval
done

multipathd will have a segfault after about 30 mins.

The reason is that mpp->hwe is accessed after hwe is already freed. In
extract_hwe_from_path func, mpp->hwe is set to pp->hwe, so they points to the
same hwe. For some reasons, pp->mpp will be set to NULL in orphan_path func.
Then, pp and hwe will be freed with (pp->mpp == NULL) in free_path func
called by ev_remove_path func. However, mpp->hwe is not set to NULL while hwe
is already freed. So, when iscsi device logs in and new path is added to mpp,
mpp->hwe will be accessed in select_pgfailback func. Finally, use-after-free
problem occurs.

The procedure details given as follows,
1.wait_dmevents thread
wait_dmevents
	->dmevent_loop
		->update_multipath
			->__setup_multipath
				->update_multipath_strings
					 -> sync_paths
						->orphan_path
2.uevqloop thread  (iscsi log out, remove path)
uevqloop
->uevent_dispatch
	->service_uevq
		->uev_remove_path
			->ev_remove_path  //pp->mpp is NULL
				->free_path(pp)
			//pp->hew are freed but mpp->hwe is not set to NULL
3.ev_remove_path  (iscsi log in, add path)
uevqloop
->uevent_dispatch
	->service_uevq
		->ev_add_path
			->select_pgfailback //mpp->hwe is accessed

Here, we will set mpp->hwe to NULL before setting pp->map to NULL in orphan_path
func.

Signed-off-by: Tianxiong Lu <lutianxiong@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 3dbbaa0..9bbe5d1 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -93,6 +93,8 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 void orphan_path(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
+	if (pp->mpp && pp->mpp->hwe == pp->hwe)
+		pp->mpp->hwe = NULL;
 	pp->mpp = NULL;
 	pp->dmstate = PSTATE_UNDEF;
 	pp->uid_attribute = NULL;
-- 
2.26.2

[PATCH 2/4] libmultipath: warn if freeing path that holds mpp->hwe

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This just adds an error message to the previous patch.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 9bbe5d1..0a73d2a 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -93,8 +93,11 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 void orphan_path(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
-	if (pp->mpp && pp->mpp->hwe == pp->hwe)
+	if (pp->mpp && pp->hwe && pp->mpp->hwe == pp->hwe) {
+		condlog(0, "BUG: orphaning path %s that holds hwe of %s",
+			pp->dev, pp->mpp->alias);
 		pp->mpp->hwe = NULL;
+	}
 	pp->mpp = NULL;
 	pp->dmstate = PSTATE_UNDEF;
 	pp->uid_attribute = NULL;
-- 
2.26.2

[PATCH 3/4] libmultipath: warn about NULL value of mpp->hwe

[mwilck]

From: Martin Wilck <mwilck@suse.com>

mpp->hwe is only accessed in propsel.c. It may become unset if
all paths of the mpp have been deleted. Access to mpp->hwe in this
case should be avoided.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/propsel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 897e48c..92b0595 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -65,7 +65,9 @@ do {									\
 	__do_set_from_vec(struct hwentry, var, (src)->hwe, dest)
 
 #define do_set_from_hwe(var, src, dest, msg)				\
-	if (__do_set_from_hwe(var, src, dest)) {			\
+	if (!src->hwe) {						\
+		condlog(0, "BUG: do_set_from_hwe called with hwe == NULL"); \
+	} else if (__do_set_from_hwe(var, src, dest)) {			\
 		origin = msg;						\
 		goto out;						\
 	}
-- 
2.26.2

[PATCH 4/4] libmultipath: fix mpp->hwe handling in sync_paths()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This is anologous to

1f96269 ("multipathd: fix mpp->hwe handling on path removal")
f6839eb ("multipathd: fix mpp->hwe handling when paths are freed")

When paths are removed from a map, we need to make sure that
mpp->hwe doesn't become stale.

Reported-by: Lixiaokeng <lixiaokeng@huawei.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 0a73d2a..0519996 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -256,6 +256,8 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 		}
 		if (!found) {
 			condlog(3, "%s dropped path %s", mpp->alias, pp->dev);
+			if (mpp->hwe == pp->hwe)
+				mpp->hwe = NULL;
 			vector_del_slot(mpp->paths, i--);
 			orphan_path(pp, "path removed externally");
 		}
@@ -263,6 +265,8 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 	update_mpp_paths(mpp, pathvec);
 	vector_foreach_slot (mpp->paths, pp, i)
 		pp->mpp = mpp;
+	if (mpp->hwe == NULL)
+		extract_hwe_from_path(mpp);
 }
 
 int
-- 
2.26.2

Re: [PATCH 0/4] Fix segfault on access to mpp->hwe

[Benjamin Marzinski]

On Mon, Jul 13, 2020 at 01:07:39PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This is in response to Lixiaokeng's post
> "master - libmultipath: fix use after free when iscsi logs in".
> 
> On top of Lixiaokeng's patch, I've added some log messages and
> a fix for mpp->hwe handling in sync_paths().
> 
> The question remains how we handle maps without paths. I believe we're good
> here, please review my assessment.
> 
> mpp->hwe is only accessed in propsel.c, via the mp_set_hwe() macro. Patch 3 of
> my series adds an error message if this happens while mpp->hwe is NULL. IMO it
> shouldn't happen because we don't check map properties for empty
> maps. Normally this is done when a map is created, and we don't create maps
> without paths.
> 
> The case where a map looses all paths during its normal lifetime and
> can't be removed (e.g. because it's busy) is already covered by the
> current code AFAICT. When a new path is re-added, we'll call adopt_paths
> and verify_paths(), which will make sure that mpp->hwe is set again
> to the pp->hwe member of the newly added path.
> 
> Reviews and comments welcome.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Regards
> Martin
> 
> Martin Wilck (3):
>   libmultipath: warn if freeing path that holds mpp->hwe
>   libmultipath: warn about NULL value of mpp->hwe
>   libmultipath: fix mpp->hwe handling in sync_paths()
> 
> lixiaokeng (1):
>   master - libmultipath: fix use after free when iscsi logs in
> 
>  libmultipath/propsel.c     | 4 +++-
>  libmultipath/structs_vec.c | 9 +++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> -- 
> 2.26.2