Re: [PATCH v4 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:
- [Medium] The logic for logging unexpected NOOP messages contradicts the commit message due to a state check that suppresses early NOOP logs.

Pre-existing issues:
- [Critical] In `ibmvfc_handle_scrq`, an unvalidated or forged `crq->ioba` token from the VIOS causes a kernel panic or arbitrary code execution because it is cast to an `evt` pointer and dereferenced before validation.
--

--- Patch [2]: [PATCH v4 2/8] ibmvfc: Add NOOP command support ---
commit 06afd3b464e63134568df7f26e9d5bc73cb9e427
Author: Dave Marquardt <davemarq@linux.ibm.com>
Subject: 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 sets the
IBMVFC_CAN_USE_NOOP_CMD capability bit during NPIV login and defines the
VFC_NOOP CRQ format handling.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index d3fd1d3437c69..a7e3b7ee06835 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -3569,6 +3571,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_VFC_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");

[Severity: Medium]
Does this logic contradict the commit message? The commit message states we
should log unexpected NOOP messages if received before VIOS advertises
support during NPIV login. 

However, checking if the host state is IBMVFC_ACTIVE appears to actively
suppress the error log if the host is still initializing during NPIV login.
Any NOOP messages received in the pre-login window would silently fail this
check and be dropped without logging.

> +		return;
> +	}
> +
> [ ... ]
> @@ -4095,7 +4105,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_VFC_NOOP)
> +		return;
> +
> +	if (unlikely(!evt)) {
> +		dev_err(vhost->dev, "Received null event\n");
>  		return;
>  	}

[Severity: Critical]
This is a pre-existing issue, but can an unvalidated or forged token from
the VIOS cause a kernel panic or arbitrary code execution here?

In ibmvfc_handle_scrq(), the token is cast directly to an evt pointer.
Before evt is validated using ibmvfc_valid_event(), it appears the code
accesses evt->queue->evt_pool.

If the token is maliciously crafted, could dereferencing evt->queue cause a
page fault? Furthermore, could a fake queue structure in guest memory bypass
the bounds check, allowing a forged event to be placed on the queue and
executing arbitrary code when evt->done(evt) is called later in
ibmvfc_drain_sub_crq()?

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