From 49eb7fcf9f75e62ac0261556370190263347c9a0 Mon Sep 17 00:00:00 2001 From: lixiaokeng Date: Tue, 1 Sep 2020 10:27:11 +0800 Subject: [PATCH] fix use after free in select_pgfailback --- ...ath-fix-use-after-free-when-iscsi-lo.patch | 84 +++++++++++++++++++ ...n-if-freeing-path-that-holds-mpp-hwe.patch | 32 +++++++ ...ath-warn-about-NULL-value-of-mpp-hwe.patch | 32 +++++++ ...h-fix-mpp-hwe-handling-in-sync_paths.patch | 44 ++++++++++ multipath-tools.spec | 12 ++- 5 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 0022-master-libmultipath-fix-use-after-free-when-iscsi-lo.patch create mode 100644 0023-libmultipath-warn-if-freeing-path-that-holds-mpp-hwe.patch create mode 100644 0024-libmultipath-warn-about-NULL-value-of-mpp-hwe.patch create mode 100644 0025-libmultipath-fix-mpp-hwe-handling-in-sync_paths.patch diff --git a/0022-master-libmultipath-fix-use-after-free-when-iscsi-lo.patch b/0022-master-libmultipath-fix-use-after-free-when-iscsi-lo.patch new file mode 100644 index 0000000..f539701 --- /dev/null +++ b/0022-master-libmultipath-fix-use-after-free-when-iscsi-lo.patch @@ -0,0 +1,84 @@ +From 7aa405ce7decd3609bcb76fa72a0ebe17ef3a635 Mon Sep 17 00:00:00 2001 +From: lixiaokeng +Date: Mon, 13 Jul 2020 13:07:40 +0200 +Subject: [PATCH 1/4] master - libmultipath: fix use after free when iscsi logs + in + +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 +Signed-off-by: lixiaokeng +Signed-off-by: Zhiqiang Liu +Signed-off-by: Martin Wilck +--- + libmultipath/structs_vec.c | 2 ++ + 1 file changed, 2 insertions(+) + +diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c +index 8137ea2..430eaad 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; +-- +1.8.3.1 + diff --git a/0023-libmultipath-warn-if-freeing-path-that-holds-mpp-hwe.patch b/0023-libmultipath-warn-if-freeing-path-that-holds-mpp-hwe.patch new file mode 100644 index 0000000..30a064a --- /dev/null +++ b/0023-libmultipath-warn-if-freeing-path-that-holds-mpp-hwe.patch @@ -0,0 +1,32 @@ +From c44d769bbca83b7dfe643712cc783db852a41b87 Mon Sep 17 00:00:00 2001 +From: Martin Wilck +Date: Mon, 13 Jul 2020 13:07:41 +0200 +Subject: [PATCH 2/4] libmultipath: warn if freeing path that holds mpp->hwe + +This just adds an error message to the previous patch. + +Signed-off-by: Martin Wilck +--- + 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 430eaad..cde4dbe 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; +-- +1.8.3.1 + diff --git a/0024-libmultipath-warn-about-NULL-value-of-mpp-hwe.patch b/0024-libmultipath-warn-about-NULL-value-of-mpp-hwe.patch new file mode 100644 index 0000000..48b3291 --- /dev/null +++ b/0024-libmultipath-warn-about-NULL-value-of-mpp-hwe.patch @@ -0,0 +1,32 @@ +From 1b21582e297d449918ea29bec2b3a0b7ba8fa84c Mon Sep 17 00:00:00 2001 +From: Martin Wilck +Date: Mon, 13 Jul 2020 13:07:42 +0200 +Subject: [PATCH 3/4] libmultipath: warn about NULL value of mpp->hwe + +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 +--- + libmultipath/propsel.c | 4 +++- + 1 file changed, 3 insertions(+), 1 deletion(-) + +diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c +index d362beb..6822827 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; \ + } +-- +1.8.3.1 + diff --git a/0025-libmultipath-fix-mpp-hwe-handling-in-sync_paths.patch b/0025-libmultipath-fix-mpp-hwe-handling-in-sync_paths.patch new file mode 100644 index 0000000..81c4eaa --- /dev/null +++ b/0025-libmultipath-fix-mpp-hwe-handling-in-sync_paths.patch @@ -0,0 +1,44 @@ +From 44292f529f6c50e1abbab04acf7f0169094bdf24 Mon Sep 17 00:00:00 2001 +From: Martin Wilck +Date: Mon, 13 Jul 2020 13:07:43 +0200 +Subject: [PATCH 4/4] libmultipath: fix mpp->hwe handling in sync_paths() + +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 +Signed-off-by: Martin Wilck +--- + libmultipath/structs_vec.c | 4 ++++ + 1 file changed, 4 insertions(+) + +diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c +index cde4dbe..ede1429 100644 +--- a/libmultipath/structs_vec.c ++++ b/libmultipath/structs_vec.c +@@ -260,6 +260,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"); + } +@@ -267,6 +269,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); + + vector_foreach_slot (mpp->pg, pgp, i) { + vector_foreach_slot (pgp->paths, pp, j) { +-- +1.8.3.1 + diff --git a/multipath-tools.spec b/multipath-tools.spec index d52d034..84e1f15 100644 --- a/multipath-tools.spec +++ b/multipath-tools.spec @@ -1,6 +1,6 @@ Name: multipath-tools Version: 0.8.4 -Release: 1 +Release: 2 Summary: Tools to manage multipath devices with the device-mapper License: GPLv2-or-later and LGPLv2+ URL: http://christophe.varoqui.free.fr/ @@ -29,6 +29,10 @@ Patch18: 0018-bugfix-clear-mpp-path-reference-when-path-is-freed-otherwis.patch Patch19: 0019-bugfix-libmultipath-fix-memory-leak-in-disassemble_map.patch Patch20: 0020-fix-find-multipath-failure.patch Patch21: 0021-change-kpartx-file-and-default-bindir.patch +Patch22:0022-master-libmultipath-fix-use-after-free-when-iscsi-lo.patch +Patch23:0023-libmultipath-warn-if-freeing-path-that-holds-mpp-hwe.patch +Patch24:0024-libmultipath-warn-about-NULL-value-of-mpp-hwe.patch +Patch25:0025-libmultipath-fix-mpp-hwe-handling-in-sync_paths.patch BuildRequires: gcc, libaio-devel, userspace-rcu-devel, device-mapper-devel >= 1.02.89 BuildRequires: libselinux-devel, libsepol-devel, readline-devel, ncurses-devel, git @@ -169,6 +173,12 @@ fi %changelog +* Tue Sep 01 2020 lixiaokeng - 0.8.4-2 +- Type:bugfix +- ID:NA +- SUG:NA +- DESC:fix use after free in select_pgfailback + * Thu Jul 16 2020 lixiaokeng - 0.8.4-1 - update to 0.8.4-1