Re: [PATCH v13 05/11] scsi: ibmvfc: add NOOP command support

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

> 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?

Okay, good idea. I'm removing the vhost parameter of
ibmvfc_handle_scrq() and adding a struct ibmvfc_queue *scrq
parameter, and updating the ibmvfc_valid_event call as suggested.

-Dave