Re: [PATCH v3 2/8] ibmvfc: Add NOOP command support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Incomplete and insecure VFC_NOOP handling in `ibmvfc_handle_scrq` leads to protocol bypass and potential data corruption.

Pre-existing issues:
- [High] Untrusted Pointer Dereference in `ibmvfc_handle_scrq` leading to kernel panic.
--

commit 026aa88f6415105823a4b94ede2214dbab9c0f42
Author: Dave Marquardt <davemarq@linux.ibm.com>

    ibmvfc: Add NOOP command support
    
    This patch adds support for receiving and recognizing VFC_NOOP
    messages from VIOS. This is done by defining the VFC_NOOP CRQ format,
    recognizing the VFC_NOOP CRQ format in the CRQ handler, and setting
    the "can use VFC_NOOP" bit in the capabilities sent during NPIV login.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index b63f41a6cfffd..110d77b9aac32 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c

[ ... ]

> @@ -4099,6 +4109,10 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
>  		return;
>  	}
>  
> +	/* Some CRQs, e.g. a VFC_NOOP command CRQ, do not have an ioba, so evt is NULL. */
> +	if (!evt)
> +		return;
> +

[Severity: High]
Does this blanket check safely handle VFC_NOOP messages?

If the VIOS sends a VFC_NOOP message with a valid, non-zero ioba that
corresponds to an active SCSI command, the sub-CRQ handler bypasses the
if (!evt) check. It will proceed to validate the event and erroneously
complete the active SCSI command using the NOOP's payload.

Additionally, this blanket check silently drops any other malformed zero-ioba
CRQs without logging them, and bypasses the IBMVFC_SUPPORT_NOOP_CMD capability
validation that is explicitly implemented in the main ibmvfc_handle_crq()
handler. 

Should the Sub-CRQ handler explicitly verify crq->format == IBMVFC_VFC_NOOP
exactly like the main handler does?

[Severity: High]
This is a pre-existing issue, but can the event validation later in
ibmvfc_handle_scrq() cause a kernel panic due to a wild pointer dereference?

The ioba is cast directly to the evt pointer. If the hypervisor or VIOS sends
a Sub-CRQ message with a malicious or uninitialized non-zero ioba value, evt
is dereferenced before it is validated:

drivers/scsi/ibmvscsi/ibmvfc.c:ibmvfc_handle_scrq() {
    ...
	if (unlikely(!ibmvfc_valid_event(&evt->queue->evt_pool, evt))) {
    ...
}

Could the struct ibmvfc_queue be passed directly to the handler so
scrq->evt_pool can be used for validation without dereferencing the
untrusted evt pointer?

>  	/* The only kind of payload CRQs we should get are responses to
>  	 * things we send. Make sure this response is to something we
>  	 * actually sent

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-ibmvfc-fpin-support-v3-0-d95b9547cf88@linux.ibm.com?part=2