[PATCH] scsi: target: fix list corruption in core_alua_free_tg_pt_gp()

[PATCH] scsi: target: fix list corruption in core_alua_free_tg_pt_gp()

[Yu Zhang]

core_alua_free_tg_pt_gp() moves every se_lun off the group being freed
and reattaches it to the default group.  For each lun it removes the
membership link under tg_pt_gp->tg_pt_gp_lock, drops that lock, and only
then takes lun->lun_tg_pt_gp_lock to call __target_attach_tg_pt_gp().

lun->lun_tg_pt_gp still points at the group being freed across that gap.
A concurrent core_alua_store_tg_pt_gp_info() holds lun->lun_tg_pt_gp_lock
across both the detach and the attach (via target_swap_tg_pt_gp()), so it
can move the same lun to a third group and re-add lun_tg_pt_gp_link to
that group's list in the window.  When the free path then runs
__target_attach_tg_pt_gp(), its bare list_add_tail() links the
already-linked node a second time, corrupting both lists; the lun's
group pointer is also overwritten and tg_pt_gp_members is decremented
twice.

Writing a new group id to a LUN's alua_tg_pt_gp attribute while another
process removes the LUN's current group (both via configfs) is enough to
trigger it:

  list_add double add: new=ffff8880055a64e0, prev=ffff8880055a64e0, next=ffff8880055a64e0.
  kernel BUG at lib/list_debug.c:35!
  RIP: 0010:__list_add_valid_or_report+0xdf/0xf0
  Call Trace:
   __target_attach_tg_pt_gp+0x9b/0x190
   core_alua_free_tg_pt_gp+0x22f/0x3a0
   config_item_put+0x89/0xa0
   configfs_rmdir+0x42f/0x5b0
   vfs_rmdir+0x22a/0x4b0

Re-check under lun->lun_tg_pt_gp_lock that the lun still references the
group being freed before reattaching it to the default group.  If a
concurrent store has already moved it, there is nothing to do: that
store holds lun->lun_tg_pt_gp_lock across its own detach and attach, so
the node stays consistently linked in the new group's list.

The store side was already made atomic in commit f9793d649c29 ("scsi:
target: Perform ALUA group changes in one step"); the free path was left
reattaching without revalidating the lun's group.

Fixes: adf653f92f38 ("target: Subsume se_port + t10_alua_tg_pt_gp_member into se_lun")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/target/target_core_alua.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 10250ac..3a085e2 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -1746,16 +1746,23 @@ void core_alua_free_tg_pt_gp(
 
 		spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
 		/*
-		 * If the passed tg_pt_gp does NOT match the default_tg_pt_gp,
-		 * assume we want to re-associate a given tg_pt_gp_mem with
-		 * default_tg_pt_gp.
+		 * tg_pt_gp_lock was dropped above, so a concurrent
+		 * core_alua_store_tg_pt_gp_info() may already have moved this
+		 * lun to another group and re-added lun_tg_pt_gp_link to that
+		 * group's list.  Reattach to the default group only while the
+		 * lun still references the group being freed; otherwise the
+		 * bare list_add_tail() in __target_attach_tg_pt_gp() would add
+		 * the already-linked node a second time and corrupt the list.
 		 */
 		spin_lock(&lun->lun_tg_pt_gp_lock);
-		if (tg_pt_gp != dev->t10_alua.default_tg_pt_gp) {
-			__target_attach_tg_pt_gp(lun,
-					dev->t10_alua.default_tg_pt_gp);
-		} else
-			rcu_assign_pointer(lun->lun_tg_pt_gp, NULL);
+		if (rcu_dereference_check(lun->lun_tg_pt_gp,
+				lockdep_is_held(&lun->lun_tg_pt_gp_lock)) == tg_pt_gp) {
+			if (tg_pt_gp != dev->t10_alua.default_tg_pt_gp)
+				__target_attach_tg_pt_gp(lun,
+						dev->t10_alua.default_tg_pt_gp);
+			else
+				rcu_assign_pointer(lun->lun_tg_pt_gp, NULL);
+		}
 		spin_unlock(&lun->lun_tg_pt_gp_lock);
 
 		spin_lock(&tg_pt_gp->tg_pt_gp_lock);
-- 
2.43.0

Re: [PATCH] scsi: target: fix list corruption in core_alua_free_tg_pt_gp()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] scsi: target: list corruption on next LUN in core_alua_free_tg_pt_gp()
- [Critical] scsi: target: use-after-free of se_lun in core_alua_free_tg_pt_gp()
--

commit 9207913678d53d80f463a158ebf713e95fda2ea5
Author: Yu Zhang <yuz08559@gmail.com>

scsi: target: fix list corruption in core_alua_free_tg_pt_gp()

This commit attempts to fix a list corruption race in
core_alua_free_tg_pt_gp() by re-checking under lun->lun_tg_pt_gp_lock
whether lun->lun_tg_pt_gp still points to the group being freed before
reattaching the LUN to the default group.

> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> index 140154d93c430..24fafcb2809fa 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -1755,16 +1755,23 @@ void core_alua_free_tg_pt_gp(
>  
>  		spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
>  		/*
> -		 * If the passed tg_pt_gp does NOT match the default_tg_pt_gp,
> -		 * assume we want to re-associate a given tg_pt_gp_mem with
> -		 * default_tg_pt_gp.
> +		 * tg_pt_gp_lock was dropped above, so a concurrent
> +		 * core_alua_store_tg_pt_gp_info() may already have moved this
> +		 * lun to another group and re-added lun_tg_pt_gp_link to that
> +		 * group's list.  Reattach to the default group only while the
> +		 * lun still references the group being freed; otherwise the
> +		 * bare list_add_tail() in __target_attach_tg_pt_gp() would add
> +		 * the already-linked node a second time and corrupt the list.
>  		 */
>  		spin_lock(&lun->lun_tg_pt_gp_lock);

[Severity: Critical]
Could this trigger a use-after-free on lun when tg_pt_gp->tg_pt_gp_lock is
dropped?

In drivers/target/target_core_alua.c:core_alua_free_tg_pt_gp(), the loop
unlinks lun from tg_pt_gp_lun_list and drops tg_pt_gp->tg_pt_gp_lock
without holding rcu_read_lock() or pinning lun->lun_ref (unlike
core_alua_queue_state_change_ua(), which pins lun via
percpu_ref_tryget_live(&lun->lun_ref) before dropping tg_pt_gp_lock).

While tg_pt_gp->tg_pt_gp_lock is dropped, a concurrent configfs unlink of
the port symlink and rmdir of the LUN directory can detach and free lun
before core_alua_free_tg_pt_gp() reaches spin_lock(&lun->lun_tg_pt_gp_lock):

  configfs_unlink()
    target_fabric_port_unlink()
      core_dev_del_lun()
        core_tpg_remove_lun()
          transport_clear_lun_ref(lun)
          target_detach_tg_pt_gp(lun)
  configfs_rmdir()
    target_fabric_drop_lun()
      target_fabric_port_release()
        call_rcu(&lun->rcu_head, target_tpg_free_lun)
          target_tpg_free_lun()
            kfree(lun)

When core_alua_free_tg_pt_gp() resumes, wouldn't spin_lock(&lun->lun_tg_pt_gp_lock)
and rcu_dereference_check(lun->lun_tg_pt_gp, ...) dereference freed
struct se_lun memory?

> -		if (tg_pt_gp != dev->t10_alua.default_tg_pt_gp) {
> -			__target_attach_tg_pt_gp(lun,
> -					dev->t10_alua.default_tg_pt_gp);
> -		} else
> -			rcu_assign_pointer(lun->lun_tg_pt_gp, NULL);
> +		if (rcu_dereference_check(lun->lun_tg_pt_gp,
> +				lockdep_is_held(&lun->lun_tg_pt_gp_lock)) == tg_pt_gp) {
> +			if (tg_pt_gp != dev->t10_alua.default_tg_pt_gp)
> +				__target_attach_tg_pt_gp(lun,
> +						dev->t10_alua.default_tg_pt_gp);
> +			else
> +				rcu_assign_pointer(lun->lun_tg_pt_gp, NULL);
> +		}
>  		spin_unlock(&lun->lun_tg_pt_gp_lock);
>  
>  		spin_lock(&tg_pt_gp->tg_pt_gp_lock);

[Severity: Critical]
What happens to the pre-fetched next pointer in list_for_each_entry_safe()
when tg_pt_gp->tg_pt_gp_lock is dropped across this window?

In drivers/target/target_core_alua.c:core_alua_free_tg_pt_gp():

	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
	list_for_each_entry_safe(lun, next,
			&tg_pt_gp->tg_pt_gp_lun_list, lun_tg_pt_gp_link) {
		list_del_init(&lun->lun_tg_pt_gp_link);
		tg_pt_gp->tg_pt_gp_members--;

		spin_unlock(&tg_pt_gp->tg_pt_gp_lock);

If tg_pt_gp_lun_list has two LUNs (lun1 and lun2), iteration 1 sets
lun = lun1, caches next = lun2, and drops tg_pt_gp->tg_pt_gp_lock.

While that lock is dropped, a concurrent core_alua_store_tg_pt_gp_info()
on lun2 can acquire lun2->lun_tg_pt_gp_lock and call
target_swap_tg_pt_gp(lun2, tg_pt_gp, tg_pt_gp_new), which moves
lun2->lun_tg_pt_gp_link into tg_pt_gp_new->tg_pt_gp_lun_list and sets
lun2->lun_tg_pt_gp = tg_pt_gp_new.

When core_alua_free_tg_pt_gp() reacquires tg_pt_gp->tg_pt_gp_lock and
advances to iteration 2 with the stale next (lun2):

1. list_for_each_entry_safe() sets next = list_next_entry(lun2, lun_tg_pt_gp_link),
   which now points into tg_pt_gp_new->tg_pt_gp_lun_list.
2. The loop body runs list_del_init(&lun2->lun_tg_pt_gp_link) and
   tg_pt_gp->tg_pt_gp_members-- without holding tg_pt_gp_new->tg_pt_gp_lock,
   corrupting tg_pt_gp_new->tg_pt_gp_lun_list.
3. Under lun2->lun_tg_pt_gp_lock, lun2->lun_tg_pt_gp == tg_pt_gp is false,
   so reattachment is skipped and lun2 is left unlinked while
   lun2->lun_tg_pt_gp still points to tg_pt_gp_new.
4. Subsequent iterations walk tg_pt_gp_new->tg_pt_gp_lun_list; because
   &tg_pt_gp_new->tg_pt_gp_lun_list != &tg_pt_gp->tg_pt_gp_lun_list, the
   loop does not terminate at tg_pt_gp_new's list head and treats
   container_of(&tg_pt_gp_new->tg_pt_gp_lun_list, struct se_lun, lun_tg_pt_gp_link)
   as a struct se_lun.

Similarly, if lun2 is concurrently detached via target_detach_tg_pt_gp(lun2)
while tg_pt_gp->tg_pt_gp_lock is dropped for lun1, list_del_init() points
lun2->lun_tg_pt_gp_link at itself, causing list_for_each_entry_safe() to
spin on lun2 indefinitely.

In addition, even with a single LUN, because list_del_init(&lun->lun_tg_pt_gp_link)
and tg_pt_gp->tg_pt_gp_members-- run before dropping tg_pt_gp->tg_pt_gp_lock
while lun->lun_tg_pt_gp still points to tg_pt_gp, won't a concurrent
core_alua_store_tg_pt_gp_info() -> target_swap_tg_pt_gp() ->
__target_detach_tg_pt_gp() in that window still decrement
tg_pt_gp->tg_pt_gp_members a second time?

霸

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019033532.73575-1-yuz08559@gmail.com?part=1