Re: [PATCH v2 02/20] scsi: ibmvfc: fix trace logging for NVMe FCP commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ibmvfc: UAF of nvmefc_fcp_req in ibmvfc_trc_start()
- [Medium] scsi: ibmvfc: trace data dropped for SCSI TMFs
--

commit d84c85cc97fa932f9084a1571f9d4d47d52e914d
Author: Tyrel Datwyler <tyreld@linux.ibm.com>
scsi: ibmvfc: fix trace logging for NVMe FCP commands

This commit fixes trace logging for NVMe FCP commands by gating SCSI-specific
field reads on evt->cmnd. It records the NVMe SQE opcode and payload length
from the v3nvme union member for NVMe FCP events.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index aa7ae81df41b1..6558ed3c67c4b 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -257,11 +258,16 @@ static void ibmvfc_trc_start(struct ibmvfc_event *evt)
>  
>  	switch (entry->fmt) {
>  	case IBMVFC_CMD_FORMAT:
> -		entry->op_code = iu->cdb[0];
>  		entry->scsi_id = be64_to_cpu(vfc_cmd->tgt_scsi_id);
> -		entry->lun = scsilun_to_int(&iu->lun);
> -		entry->tmf_flags = iu->tmf_flags;
> -		entry->u.start.xfer_len = be32_to_cpu(iu->xfer_len);
> +		if (evt->cmnd) {
> +			entry->op_code = iu->cdb[0];
> +			entry->lun = scsilun_to_int(&iu->lun);
> +			entry->tmf_flags = iu->tmf_flags;
> +			entry->u.start.xfer_len = be32_to_cpu(iu->xfer_len);

[Severity: Medium]
Will this drop trace data for SCSI Task Management Functions?

When an internally generated SCSI TMF is processed, evt->cmnd is initialized
to NULL. Since the extraction of LUN and TMF flags is now strictly gated on
evt->cmnd, TMFs will be skipped entirely leaving these trace fields zeroed out.

> +		} else if (evt->fcp_req) {
> +			entry->op_code = vfc_cmd->v3nvme.iu.sqe.common.opcode;
> +			entry->u.start.xfer_len = evt->fcp_req->payload_length;

[Severity: High]
Can this result in a use-after-free on the NVMe FCP request?

The command has already been submitted to the hardware at this point. In
ibmvfc_send_event(), the queue lock is dropped before calling
ibmvfc_trc_start():

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_send_event() {
	...
	spin_unlock_irqrestore(&evt->queue->l_lock, flags);
	ibmvfc_trc_start(evt);
}

Because the lock is dropped, the hardware completion can race and process
the response, calling evt->done(). This hands the NVMe FCP request back to
the block layer where it is completed and freed. Meanwhile, this thread will
locklessly dereference evt->fcp_req->payload_length here, accessing the freed
memory.

> +		}
>  		break;
>  	case IBMVFC_MAD_FORMAT:

[ ... ]

> @@ -294,15 +301,19 @@ static void ibmvfc_trc_end(struct ibmvfc_event *evt)
>  
>  	switch (entry->fmt) {
>  	case IBMVFC_CMD_FORMAT:
> -		entry->op_code = iu->cdb[0];
>  		entry->scsi_id = be64_to_cpu(vfc_cmd->tgt_scsi_id);
> -		entry->lun = scsilun_to_int(&iu->lun);
> -		entry->tmf_flags = iu->tmf_flags;
>  		entry->u.end.status = be16_to_cpu(vfc_cmd->status);
>  		entry->u.end.error = be16_to_cpu(vfc_cmd->error);
> -		entry->u.end.fcp_rsp_flags = rsp->flags;
> -		entry->u.end.rsp_code = rsp->data.info.rsp_code;
> -		entry->u.end.scsi_status = rsp->scsi_status;
> +		if (evt->cmnd) {
> +			entry->op_code = iu->cdb[0];
> +			entry->lun = scsilun_to_int(&iu->lun);
> +			entry->tmf_flags = iu->tmf_flags;
> +			entry->u.end.fcp_rsp_flags = rsp->flags;
> +			entry->u.end.rsp_code = rsp->data.info.rsp_code;
> +			entry->u.end.scsi_status = rsp->scsi_status;

[Severity: Medium]
Will this also drop trace data in the end trace for SCSI TMFs?

Similarly to the start trace, since evt->cmnd is NULL for SCSI TMFs, the
response code, FCP response flags, and SCSI status will be left unrecorded
for these events.

> +		} else if (evt->fcp_req) {
> +			entry->op_code = vfc_cmd->v3nvme.iu.sqe.common.opcode;
> +		}
>  		break;
>  	case IBMVFC_MAD_FORMAT:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919013306.2948028-1-tyreld@linux.ibm.com?part=2