Re: [PATCH v7 2/8] scsi: 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:
- [Medium] The code suppresses logging of unexpected NOOP messages received before NPIV login completes, directly contradicting the commit message claim of logging early messages.

Pre-existing issues:
- [High] Dereferencing untrusted pointer `evt->queue` before validation in `ibmvfc_handle_scrq` can lead to a kernel panic or out-of-bounds read if the partner sends a malicious or corrupted non-NULL correlation token.
--

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

scsi: ibmvfc: Add NOOP command support

This commit adds support for VFC_NOOP messages from VIOS to enable keep-alive
functionality between the client and server. It defines the VFC_NOOP CRQ
format, adds handling in the CRQ handlers, and sets the NPIV login capability
bit.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 78be00d31a064..e972e4ec5be00 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3693,6 +3695,14 @@ static void ibmvfc_handle_crq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost,
>  	if (crq->format == IBMVFC_ASYNC_EVENT)
>  		return;
>  
> +	if (crq->format == IBMVFC_NOOP) {
> +		if (vhost->state == IBMVFC_ACTIVE &&
> +		    !ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NOOP_CMD))
> +			dev_err_ratelimited(vhost->dev,
> +					    "Received unexpected NOOP command from partner\n");
> +		return;
> +	}
> +

[Severity: Medium]
The commit message states the intent is to "Log unexpected NOOP messages if
received before VIOS advertises support during NPIV login." Does this
condition match that intent?

In drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_handle_crq(), the code checks:
if (vhost->state == IBMVFC_ACTIVE && !ibmvfc_check_caps(...))

By requiring the host state to be IBMVFC_ACTIVE, this appears to suppress
logging of unexpected NOOP messages received during the IBMVFC_INITIALIZING
state, and only logs them after NPIV login has completed.

>  	/* 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
[ ... ]
> @@ -4223,7 +4233,15 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
>  	case IBMVFC_CRQ_XPORT_EVENT:
>  		return;
>  	default:
> -		dev_err(vhost->dev, "Got and invalid message type 0x%02x\n", crq->valid);
> +		dev_err(vhost->dev, "Got an invalid message type 0x%02x\n", crq->valid);
> +		return;
> +	}
> +
> +	if (crq->format == IBMVFC_NOOP)
> +		return;
> +
> +	if (unlikely(!evt)) {
> +		dev_err(vhost->dev, "Received null event\n");
>  		return;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to an
out-of-bounds read or kernel panic if the VIOS partner sends a corrupted or
malicious non-NULL correlation token?

Just below this new null check in
drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_handle_scrq(), the untrusted
evt pointer is dereferenced prior to validation:

    if (unlikely(!ibmvfc_valid_event(&evt->queue->evt_pool, evt))) {

Could a malformed CRQ payload with a corrupted crq->ioba token cause a
crash when accessing evt->queue before verifying that evt is a legitimate
pointer from the event pool?

Passing the scrq pointer from ibmvfc_drain_sub_crq() instead of vhost might
be a way to avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-ibmvfc-fpin-support-v7-0-77d0b9809080@linux.ibm.com?part=2