Re: [PATCH v7 2/8] 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 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] The code suppresses logging of unexpected NOOP messages received before NPIV login completes, directly contradicting the commit message claim of logging early messages.
>
> Pre-existing issues:
> - [High] Dereferencing untrusted pointer `evt->queue` before validation in `ibmvfc_handle_scrq` can lead to a kernel panic or out-of-bounds read if the partner sends a malicious or corrupted non-NULL correlation token.
> --
>
> commit a2f1ce40ef9fb5e6c86a16b1ac92eb6c526f0a48
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: 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 defines the VFC_NOOP CRQ
> format, adds handling in the CRQ handlers, and sets the NPIV login capability
> bit.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 78be00d31a064..e972e4ec5be00 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -3693,6 +3695,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_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;
>> +	}
>> +
>
> [Severity: Medium]
> The commit message states the intent is to "Log unexpected NOOP messages if
> received before VIOS advertises support during NPIV login." Does this
> condition match that intent?

No, this does not match. I will rewrite the commit message to reflect reality.

> In drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_handle_crq(), the code checks:
> if (vhost->state == IBMVFC_ACTIVE && !ibmvfc_check_caps(...))
>
> By requiring the host state to be IBMVFC_ACTIVE, this appears to suppress
> logging of unexpected NOOP messages received during the IBMVFC_INITIALIZING
> state, and only logs them after NPIV login has completed.
>
>>  	/* 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
> [ ... ]
>> @@ -4223,7 +4233,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_NOOP)
>> +		return;
>> +
>> +	if (unlikely(!evt)) {
>> +		dev_err(vhost->dev, "Received null event\n");
>>  		return;
>>  	}
>
> [Severity: High]
> This isn't a bug introduced by this patch, but can this lead to an
> out-of-bounds read or kernel panic if the VIOS partner sends a corrupted or
> malicious non-NULL correlation token?

We trust the VIOS partner, so I will not update this.

-Dave