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

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

[Jia Jia]

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

prot_bytes comes from virtio_scsi_cmd_req_pi.  With
VIRTIO_SCSI_F_T10_PI negotiated, vhost reads that header and does not
check that the CDB is a READ or WRITE.  sbc_check_prot() replaces
data_length from the CDB only when the protect field is non-zero.
target_cmd_size_check() leaves a write's data_length unchanged when the
CDB size is smaller, so a command that does not get that replacement
keeps a data_length larger than the data sgl.

The KASAN report is a MODE SELECT whose header had pi_bytesout set.
The data sgl held only the parameter list.  spc_emulate_modeselect()
walked data_length and read the next physical page.

Pass the data sgl length.  Protected READ and WRITE still take their
length from the CDB in sbc_check_prot().

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>
---
v2:
  - Drop the claim that target used to subtract prot_length.
    9f977ef7b671 cites 14ef9200, which is not in the tree
  - prot_bytes comes from virtio_scsi_cmd_req_pi

 drivers/vhost/scsi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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:
 		/*
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] vhost-scsi: missing validation of prot_bytes alignment allows OOB access
--

commit a2683cd412935b122ac2e93ce16af9655a892639
Author: Jia Jia <physicalmtea@gmail.com>

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

This commit updates the expected data length passed to the target core
by omitting the protection information (PI) bytes. It adjusts the length
so that protected read and write operations take their length directly
from the CDB, preventing issues when the data scatterlist is smaller
than the total length.

> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 885fac5ed042e..8b32caa7ec294 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);

[Severity: Critical]
Does this code allow an out-of-bounds access when prot_bytes is not aligned?

In vhost_scsi_handle_vq(), prot_bytes is extracted from the virtqueue element:

    prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesin);

There doesn't appear to be validation ensuring it is a multiple of the 8-byte
T10 PI tuple size before it dictates the length of the protection scatterlist.

When the target core later processes this command, functions like
sbc_dif_generate() in drivers/target/target_core_sbc.c loop over the
scatterlist length using 8-byte increments:

    for (j = 0; j < psg->length; j += sizeof(*sdt)) {

If psg->length is not a multiple of 8, does the last iteration read or write
past the end of the mapped scatterlist element? A malicious guest could provide
an unaligned pi_bytesin or pi_bytesout length, potentially causing an
out-of-bounds access into an adjacent physical page on the host.

>  		ret = 0;
>  err:

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

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

[Jia Jia]

>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] vhost-scsi: missing validation of prot_bytes alignment allows OOB access
> --
>
> commit a2683cd412935b122ac2e93ce16af9655a892639
> Author: Jia Jia <physicalmtea@gmail.com>
>
> vhost-scsi: do not count PI bytes in the data length
>
> This commit updates the expected data length passed to the target core
> by omitting the protection information (PI) bytes. It adjusts the length
> so that protected read and write operations take their length directly
> from the CDB, preventing issues when the data scatterlist is smaller
> than the total length.
>
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 885fac5ed042e..8b32caa7ec294 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);
>
> [Severity: Critical]
> Does this code allow an out-of-bounds access when prot_bytes is not aligned?
>
> In vhost_scsi_handle_vq(), prot_bytes is extracted from the virtqueue element:
>
>     prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesin);
>
> There doesn't appear to be validation ensuring it is a multiple of the 8-byte
> T10 PI tuple size before it dictates the length of the protection scatterlist.
>
> When the target core later processes this command, functions like
> sbc_dif_generate() in drivers/target/target_core_sbc.c loop over the
> scatterlist length using 8-byte increments:
>
>     for (j = 0; j < psg->length; j += sizeof(*sdt)) {
>
> If psg->length is not a multiple of 8, does the last iteration read or write
> past the end of the mapped scatterlist element? A malicious guest could provide
> an unaligned pi_bytesin or pi_bytesout length, potentially causing an
> out-of-bounds access into an adjacent physical page on the host.
>

This issue is not introduced by the current patch. I have previously
sent related patches to fix similar issues. I personally believe that
this kind of issue should not be fixed in the transport layer.

https://lore.kernel.org/all/20261006093338.27342-5-physicalmtea@gmail.com/
https://lore.kernel.org/all/20261006093338.27342-6-physicalmtea@gmail.com/
https://lore.kernel.org/all/20261006093338.27342-7-physicalmtea@gmail.com/

> >               ret = 0;
> >  err:
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261011014803.100454-1-physicalmtea@gmail.com?part=1

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

[Michael S. Tsirkin]

On Sun, Oct 11, 2026 at 09:48:03AM +0800, Jia Jia wrote:
> vhost_scsi_handle_vq() moves pi_bytesout or pi_bytesin into the
> protection sgl and subtracts those bytes from exp_data_len.  The length
> passed to TCM is still exp_data_len + prot_bytes.
> 
> prot_bytes comes from virtio_scsi_cmd_req_pi.  With
> VIRTIO_SCSI_F_T10_PI negotiated, vhost reads that header and does not
> check that the CDB is a READ or WRITE.  sbc_check_prot() replaces
> data_length from the CDB only when the protect field is non-zero.
> target_cmd_size_check() leaves a write's data_length unchanged when the
> CDB size is smaller, so a command that does not get that replacement
> keeps a data_length larger than the data sgl.
> 
> The KASAN report is a MODE SELECT whose header had pi_bytesout set.
> The data sgl held only the parameter list.  spc_emulate_modeselect()
> walked data_length and read the next physical page.
> 
> Pass the data sgl length.  Protected READ and WRITE still take their
> length from the CDB in sbc_check_prot().
> 
> 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>
> ---
> v2:
>   - Drop the claim that target used to subtract prot_length.
>     9f977ef7b671 cites 14ef9200, which is not in the tree
>   - prot_bytes comes from virtio_scsi_cmd_req_pi


you can mention the other patches you sent fixing
security maybe this will calm down sashiko.

> 
>  drivers/vhost/scsi.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> 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().

this is barely readable - could you improve pls? even if that makes it
a bit more verbose.




> +		 */
>  		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:
>  		/*
> -- 
> 2.53.0

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

[Jia Jia]

>
> On Sun, Oct 11, 2026 at 09:48:03AM +0800, Jia Jia wrote:
> > vhost_scsi_handle_vq() moves pi_bytesout or pi_bytesin into the
> > protection sgl and subtracts those bytes from exp_data_len.  The length
> > passed to TCM is still exp_data_len + prot_bytes.
> >
> > prot_bytes comes from virtio_scsi_cmd_req_pi.  With
> > VIRTIO_SCSI_F_T10_PI negotiated, vhost reads that header and does not
> > check that the CDB is a READ or WRITE.  sbc_check_prot() replaces
> > data_length from the CDB only when the protect field is non-zero.
> > target_cmd_size_check() leaves a write's data_length unchanged when the
> > CDB size is smaller, so a command that does not get that replacement
> > keeps a data_length larger than the data sgl.
> >
> > The KASAN report is a MODE SELECT whose header had pi_bytesout set.
> > The data sgl held only the parameter list.  spc_emulate_modeselect()
> > walked data_length and read the next physical page.
> >
> > Pass the data sgl length.  Protected READ and WRITE still take their
> > length from the CDB in sbc_check_prot().
> >
> > 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>
> > ---
> > v2:
> >   - Drop the claim that target used to subtract prot_length.
> >     9f977ef7b671 cites 14ef9200, which is not in the tree
> >   - prot_bytes comes from virtio_scsi_cmd_req_pi
>
>
> you can mention the other patches you sent fixing
> security maybe this will calm down sashiko.
>

Will do, thanks.

> >
> >  drivers/vhost/scsi.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > 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().
>
> this is barely readable - could you improve pls? even if that makes it
> a bit more verbose.
>

Thanks for the reminder. I'll improve it.

>
>
>
> > +              */
> >               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:
> >               /*
> > --
> > 2.53.0
>