Re: [PATCH v9 2/9] scsi: ibmvfc: add NOOP command support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Untrusted hypervisor input (`crq->ioba`) is dereferenced prior to validation, leading to a potential unmapped page fault (kernel panic).
--

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

scsi: ibmvfc: add NOOP command support

This commit adds support for handling IBMVFC_NOOP format CRQ messages from
the VIOS partner and advertises this capability during login. It updates
CRQ handlers to ignore NOOP messages while logging rate-limited errors if
the partner did not advertise support. Finally, it fixes a typo and adds a
null check for event pointers in ibmvfc_handle_scrq().

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 5259a80958ce1..6b2eb82ceb98d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -4225,7 +4235,20 @@ 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) {
> +		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;
> +	}
> +
> +	if (unlikely(!evt)) {
> +		dev_err(vhost->dev, "Received null event\n");
>  		return;
>  	}
>  
>  	/* 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
>  	 */
>  	if (unlikely(!ibmvfc_valid_event(&evt->queue->evt_pool, evt))) {

[Severity: High]
This isn't a bug introduced by this patch, but does ibmvfc_handle_scrq()
dereference the unvalidated evt pointer?

Since evt is derived from an untrusted hypervisor value (crq->ioba), if the
hypervisor sends an invalid non-null pointer, evaluating &evt->queue->evt_pool
will dereference evt->queue before ibmvfc_valid_event() can check if evt
falls within the legitimate event pool bounds. Can this cause an unmapped
page fault and kernel panic?

Should the implementation bypass the untrusted pointer entirely when locating
the event pool, for instance by passing the actual scrq object from the
caller?

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