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

[Tyrel Datwyler <tyreld@linux.ibm.com>]

On 9/10/26 7:31 PM, sashiko-bot@kernel.org wrote:
> 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?
> 

We trust the hypervisor/VIOS to provide a valid IOBA to the relevant evt structure.

-Tyrel