Re: [PATCH 1/2] scsi: target: alua: Don't wait for a grace period when attaching a LUN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] target: out-of-bounds write in
  target_emulate_report_target_port_groups() [1]
--

commit 2f412f4b4c4324ed81aac0037066698113367ec3
Author: Qiliang Yuan <qiliang.yuan@linux.dev>

scsi: target: alua: Don't wait for a grace period when attaching a LUN

This commit removes the synchronize_rcu() call from target_attach_tg_pt_gp()
when attaching a newly created LUN to the default ALUA target port group.

> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> index 140154d93c430..35d36cd36db6c 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -1833,7 +1833,6 @@ void target_attach_tg_pt_gp(struct se_lun *lun,
>  	spin_lock(&lun->lun_tg_pt_gp_lock);
>  	__target_attach_tg_pt_gp(lun, tg_pt_gp);
>  	spin_unlock(&lun->lun_tg_pt_gp_lock);
> -	synchronize_rcu();
>  }
>  
>  static void __target_detach_tg_pt_gp(struct se_lun *lun,

[Severity: Critical]
This problem wasn't introduced by this patch, but can concurrent calls to
target_attach_tg_pt_gp() and target_emulate_report_target_port_groups() cause
an out-of-bounds write to buf[] [1]?

drivers/target/target_core_alua.c:target_emulate_report_target_port_groups() {
    ...
		if ((off + 8 + (tg_pt_gp->tg_pt_gp_members * 4)) >
		     cmd->data_length) {
			rd_len += 8 + (tg_pt_gp->tg_pt_gp_members * 4);
			continue;
		}
    ...
		spin_lock(&tg_pt_gp->tg_pt_gp_lock);
		list_for_each_entry(lun, &tg_pt_gp->tg_pt_gp_lun_list,
				lun_tg_pt_gp_link) {
			off += 2; /* Skip over Obsolete */
			put_unaligned_be16(lun->lun_tpg->tpg_rtpi, &buf[off]);
			off += 2;
			rd_len += 4;
		}
		spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
    ...
}

target_emulate_report_target_port_groups() checks whether the descriptors for
a port group fit within cmd->data_length using tg_pt_gp->tg_pt_gp_members
without holding tg_pt_gp->tg_pt_gp_lock:

CPU0                                  CPU1
target_emulate_report_target_port_groups()
  if ((off + 8 +
       (tg_pt_gp->tg_pt_gp_members * 4)) >
      cmd->data_length)
                                      target_attach_tg_pt_gp()
                                        __target_attach_tg_pt_gp()
                                          spin_lock(&tg_pt_gp->tg_pt_gp_lock)
                                          list_add_tail(...,
                                            &tg_pt_gp->tg_pt_gp_lun_list)
                                          tg_pt_gp->tg_pt_gp_members++
                                          spin_unlock(&tg_pt_gp->tg_pt_gp_lock)
  spin_lock(&tg_pt_gp->tg_pt_gp_lock)
  list_for_each_entry(lun, ...)
    put_unaligned_be16(..., &buf[off])  <-- out-of-bounds write

When a concurrent configfs operation attaches a LUN to the same port group
between the size check and acquiring tg_pt_gp->tg_pt_gp_lock,
tg_pt_gp_lun_list grows past the validated count and the loop writes past the
end of the mapped command buffer.

[1] https://sashiko.dev/#/bug/linux-9cff51b1-852e-4bef-94e2-6d7c000ddc21

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-bug-target-alua-lun-gp-v1-0-127a46efdae9@linux.dev?part=1