Re: [PATCH v13 05/11] 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:
- [High] scsi: ibmvfc: wild pointer dereference in ibmvfc_handle_scrq() during evt validation
--

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

scsi: ibmvfc: add NOOP command support

Add support for handling IBMVFC_NOOP format CRQ messages from the
VIOS partner. Advertise the client's ability to handle NOOP commands
by setting the IBMVFC_CAN_USE_NOOP_CMD capability in ibmvfc_set_login_info().
Handle and ignore IBMVFC_NOOP format messages in both ibmvfc_handle_crq()
and ibmvfc_handle_scrq().

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index ac2571e9e1eb9..161d4679d7026 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]
Can this code dereference a wild pointer if the VIOS partner or hypervisor 
sends an arbitrary, non-zero garbage token in crq->ioba?

At the top of ibmvfc_handle_scrq(), crq->ioba is cast to the evt pointer.
While this patch adds a check to prevent a NULL pointer dereference, if
crq->ioba is non-zero garbage, wouldn't evaluating evt->queue dereference
that untrusted pointer before ibmvfc_valid_event() has a chance to safely 
validate it?

Looking at ibmvfc_handle_crq(), it avoids dereferencing the event entirely 
during validation by checking against the host pool:

	if (unlikely(!ibmvfc_valid_event(&vhost->crq.evt_pool, evt))) {

Could we do something similar here by passing the valid scrq directly into 
ibmvfc_handle_scrq() so it can safely use &scrq->evt_pool?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-ibmvfc-fpin-support-v13-0-b816abfa4fcc@linux.ibm.com?part=5