[PATCH] vhost-scsi: do not count PI bytes in the data length

[PATCH] vhost-scsi: do not count PI bytes in the data length

[Jia Jia]

vhost_scsi_handle_vq() moves pi_bytesout and pi_bytesin into the
protection sgl and subtracts them from exp_data_len.  The length passed
to TCM is still exp_data_len + prot_bytes.

That sum matched an older target path which subtracted prot_length
again.  sbc_check_prot() now replaces data_length from the CDB only
when the protect field is non-zero, and only for READ and WRITE.
target_cmd_size_check() leaves a write's data_length unchanged when the
CDB size is smaller.

MODE SELECT, SET TARGET PORT GROUPS and UNMAP then use data_length as
the size of t_data_sg.  The PI bytes are not in that sgl.  A parameter
list that ends on a page is followed by a read of the next physical
page.

Pass the data sgl length.  Protected READ and WRITE still take their
length from the CDB.

KASAN reports:

  BUG: KASAN: use-after-free in spc_emulate_modeselect+0x145/0x380 [target_core_mod]
  Read of size 1

  spc_emulate_modeselect
  __target_execute_cmd
  target_execute_cmd
  vhost_scsi_write_pending
  transport_generic_new_cmd
  __target_submit
  target_queued_submit_work
  process_one_work
  worker_thread
  kthread
  ret_from_fork
  ret_from_fork_asm

Fixes: 9f977ef7b671 ("vhost-scsi: Include prot_bytes into expected data transfer length")
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 4f8c0260bc9e..4bd29c2ceed9 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1534,9 +1534,13 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		 * vhost_scsi_queue_data_in() and vhost_scsi_queue_status()
 		 */
 		cmd->tvc_vq_desc = vc.head;
+		/*
+		 * exp_data_len is the data sgl. prot_bytes already sit in
+		 * the protection sgl. Protected READ and WRITE replace
+		 * data_length from the CDB in sbc_check_prot().
+		 */
 		vhost_scsi_target_queue_cmd(nexus, cmd, cdb, lun, task_attr,
-					    data_direction,
-					    exp_data_len + prot_bytes);
+					    data_direction, exp_data_len);
 		ret = 0;
 err:
 		/*

Re: [PATCH] vhost-scsi: do not count PI bytes in the data length

[Mike Christie]

On 10/6/26 3:21 AM, Jia Jia wrote:
> vhost_scsi_handle_vq() moves pi_bytesout and pi_bytesin into the
> protection sgl and subtracts them from exp_data_len.  The length passed
> to TCM is still exp_data_len + prot_bytes.
> 
> That sum matched an older target path which subtracted prot_length
> again.  sbc_check_prot() now replaces data_length from the CDB only

What patch changed this? Did all the drivers except vhost-scsi get fixed?

> when the protect field is non-zero, and only for READ and WRITE.
> target_cmd_size_check() leaves a write's data_length unchanged when the
> CDB size is smaller.
> 
> MODE SELECT, SET TARGET PORT GROUPS and UNMAP then use data_length as
> the size of t_data_sg.  The PI bytes are not in that sgl.  A parameter

Can you have prot_bytes > 0 with those commands?

Re: [PATCH] vhost-scsi: do not count PI bytes in the data length

[Jia Jia]

>
> On 10/6/26 3:21 AM, Jia Jia wrote:
> > vhost_scsi_handle_vq() moves pi_bytesout and pi_bytesin into the
> > protection sgl and subtracts them from exp_data_len.  The length passed
> > to TCM is still exp_data_len + prot_bytes.
> >
> > That sum matched an older target path which subtracted prot_length
> > again.  sbc_check_prot() now replaces data_length from the CDB only
>
> What patch changed this? Did all the drivers except vhost-scsi get fixed?
>

I had that wrong.  9f977ef7b671 says target would subtract prot_length
and points at 14ef9200.  That commit is not in the tree, and the target
code never subtracts prot_length.  The in-tree behavior is
e2a4f55c6498: if protect is non-zero, sbc_check_prot() sets
data_length from the CDB.  The vhost add has been there since
9f977ef7b671, in the same pull.  Nothing later changed it.

> > when the protect field is non-zero, and only for READ and WRITE.
> > target_cmd_size_check() leaves a write's data_length unchanged when the
> > CDB size is smaller.
> >
> > MODE SELECT, SET TARGET PORT GROUPS and UNMAP then use data_length as
> > the size of t_data_sg.  The PI bytes are not in that sgl.  A parameter
>
> Can you have prot_bytes > 0 with those commands?

I did not describe this clearly.  MODE SELECT is not where prot_bytes
comes from.
With VIRTIO_SCSI_F_T10_PI negotiated, pi_bytesout can be set in
virtio_scsi_cmd_req_pi, and vhost does not check that the CDB is a
READ or WRITE. .
A MODE SELECT sent with that field set hits this.  That is what I ran.