> > 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 <[email protected]> > > 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/[email protected]/ https://lore.kernel.org/all/[email protected]/ https://lore.kernel.org/all/[email protected]/ > > ret = 0; > > err: > > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/[email protected]?part=1
