Re: [GIT PULL] SCSI fixes for 7.4-rc1+

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] scsi: target: premature ACL publication in
  core_tpg_check_initiator_node_acl()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] scsi: target: tcmu: TOCTOU race on max_blocks in configfs param
  setters [1]
--

commit f9d11da432d3548975ae975a55e7c7c925a3b8f1
Author: James Bottomley <James.Bottomley@HansenPartnership.com>

SCSI fixes for 7.4-rc1+

This merge commit collects scsi driver, core, bsg, ufs, and target fixes,
including target acl and tcmu parameter handling updates.

[ ... ]

> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index ccff9f6e48266..e615e5e304273 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -266,6 +266,15 @@ struct se_node_acl *core_tpg_check_initiator_node_acl(
>  	kref_get(&acl->acl_kref);
>  	acl->dynamic_node_acl = 1;
>  
> +	/*
> +	 * Publish the dynamic NodeACL on tpg->acl_node_list before creating
> +	 * its demo-mode MappedLUN dev entries below, so that a concurrent LUN
> +	 * teardown sweep (core_clear_lun_from_tpg(), which walks acl_node_list)
> +	 * can observe and disable those dev entries.  Otherwise a dev entry can
> +	 * be left on an se_lun->lun_deve_list that is about to be freed.
> +	 */
> +	target_add_node_acl(acl);
> +
>  	/*
>  	 * Here we only create demo-mode MappedLUNs from the active
>  	 * TPG LUNs if the fabric is not explicitly asking for
> @@ -275,7 +284,6 @@ struct se_node_acl *core_tpg_check_initiator_node_acl(
>  	    (tpg->se_tpg_tfo->tpg_check_demo_mode_login_only(tpg) != 1))
>  		core_tpg_add_node_to_devs(acl, tpg, NULL);
>  
> -	target_add_node_acl(acl);
>  	return acl;
>  }
>  EXPORT_SYMBOL(core_tpg_check_initiator_node_acl);

[Severity: High]
Does moving target_add_node_acl() before core_tpg_add_node_to_devs()
publish acl on tpg->acl_node_list before its demo-mode lun entries and
session are attached?

target_add_node_acl() links acl onto tpg->acl_node_list and drops
tpg->acl_node_mutex while acl->lun_entry_hlist and acl->acl_sess_list are
still empty.  A concurrent login from the same initiator can then look up
acl via core_tpg_get_initiator_node_acl(), see an unpopulated lun map, and
if that second session tears down before the first session registers on
acl->acl_sess_list (when tpg_check_demo_mode_cache() is false),
transport_free_session() sees an empty acl_sess_list and unlinks and drops
acl:

CPU0                                  CPU1
core_tpg_check_initiator_node_acl()
  target_add_node_acl(acl)
                                      core_tpg_check_initiator_node_acl()
                                        core_tpg_get_initiator_node_acl()
                                          -> returns acl
                                      ...
                                      transport_free_session()
                                        list_empty(&acl->acl_sess_list)
                                        acl->dynamic_stop = true
                                        list_del_init(&acl->acl_list)
                                        target_put_nacl(acl) (x2)
  core_tpg_add_node_to_devs(acl, ...)
  ...
  transport_free_session()
    target_put_nacl(acl)              <-- refcount underflow / use-after-free

Because CPU0's core_tpg_add_node_to_devs() runs after acl has been
unlinked from tpg->acl_node_list, its new se_dev_entry objects are also
missed by core_clear_lun_from_tpg() on lun removal, leaving dangling
entries on se_lun->lun_deve_list.

In addition, a concurrent core_dev_add_lun() walking tpg->acl_node_list
under tpg->acl_node_mutex can call core_tpg_add_node_to_devs(acl, tpg,
lun) before CPU0 calls core_tpg_add_node_to_devs(acl, tpg, NULL),
replacing the first demo-mode se_dev_entry in
core_enable_device_list_for_node() and leaking its per-cpu new->stats when
orig is freed via kfree_rcu().  Likewise, core_tpg_add_initiator_node_acl()
can find acl and clear acl->dynamic_node_acl = 0 before
core_tpg_add_node_to_devs() runs, attaching demo-mode lun entries to an
explicit acl.

> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 425ad1246a759..07b5146781767 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -2524,6 +2524,11 @@ static int tcmu_set_data_pages_per_blk(struct tcmu_dev *udev, substring_t *arg)
>  		return ret;
>  	}
>  
> +	if (val <= 0) {
> +		pr_err("Invalid data_pages_per_blk %d.\n", val);
> +		return -EINVAL;
> +	}
> +
>  	if (val > TCMU_MBS_TO_PAGES(udev->data_area_mb)) {
>  		pr_err("Invalid data_pages_per_blk %d: greater than max_data_area_mb %d -> %zd pages).\n",
>  		       val, udev->data_area_mb,

[Severity: High]
This problem wasn't introduced by this patch, but can concurrent configfs
writes to tcmu_set_data_pages_per_blk() and tcmu_set_max_blocks_param()
race [1] to set udev->max_blocks to 0?

Both helpers validate udev->data_area_mb and udev->data_pages_per_blk
before acquiring udev->cmdr_lock:

CPU0                                  CPU1
tcmu_set_data_pages_per_blk()         tcmu_set_max_blocks_param()
  val <= TCMU_MBS_TO_PAGES(
           udev->data_area_mb)
                                        TCMU_MBS_TO_PAGES(val) >=
                                          pages_per_blk
                                        mutex_lock(&udev->cmdr_lock)
                                        udev->data_area_mb = val
                                        udev->max_blocks = ...
                                        mutex_unlock(&udev->cmdr_lock)
  mutex_lock(&udev->cmdr_lock)
  udev->data_pages_per_blk = val
  udev->max_blocks =
    TCMU_MBS_TO_PAGES(
      udev->data_area_mb) / val       <-- evaluates to 0
  mutex_unlock(&udev->cmdr_lock)

When tcmu_configure_device() later allocates udev->data_bitmap with
bitmap_zalloc(), udev->max_blocks == 0 returns a zero-size pointer that
faults on subsequent bitmap operations.  In the reverse interleaving,
tcmu_set_max_blocks_param() computes udev->max_blocks from a stale
pages_per_blk snapshot before udev->data_pages_per_blk is increased,
leaving udev->max_blocks * udev->data_blk_size larger than
udev->data_area_mb.

Checking and updating both parameters under udev->cmdr_lock would avoid
the race.

[1] https://sashiko.dev/#/bug/linux-873290f2-6e60-4ebc-a4fd-858ba6bf6b4b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/49923933505639f8c35894464a0a8c57773d4877.camel@HansenPartnership.com?part=1