Re: [PATCH v15 03/11] scsi: ibmvfc: fix potential NULL pointer dereference on failed queue allocation

[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:
> - [Medium] scsi: ibmvfc: scrq->cookie not cleared on H_REG_SUB_CRQ failure in ibmvfc_register_channel()
> --
>
> commit 09a8086d40f44887491493f172cd7dd44285d18d
> Author: Tyrel Datwyler <tyreld@linux.ibm.com>
>
> scsi: ibmvfc: fix potential NULL pointer dereference on failed queue allocation
>
> This commit guards ibmvfc_deregister_channel() against calling H_FREE_SUB_CRQ
> on an unregistered sub-CRQ queue and against zeroing an unallocated message
> buffer. It also adds a KUnit test suite to verify the deregistration guards.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 145d559041f12..0fe6b6f4260ba 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> @@ -6449,12 +6449,24 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>>  	do {
>>  		hcall_rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
>>  	} while (hcall_rc == H_BUSY || H_IS_LONG_BUSY(hcall_rc));
>> +	scrq->cookie = 0;
>>  reg_failed:
>>  	LEAVE;
>>  	return rc;
>>  }
>
> [Severity: Medium]
> Should scrq->cookie = 0 be placed after the reg_failed label (or should
> h_reg_sub_crq() avoid writing *cookie on error)?
>
> Looking at h_reg_sub_crq() in drivers/scsi/ibmvscsi/ibmvfc-core.c, it
> unconditionally writes retbuf[0] into *cookie regardless of the return code:
>
> static long h_reg_sub_crq(unsigned long unit_address, unsigned long ioba,
> 			  unsigned long length, unsigned long *cookie,
> 			  unsigned long *irq)
> {
> 	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
> 	long rc;
>
> 	rc = plpar_hcall(H_REG_SUB_CRQ, retbuf, unit_address, ioba, length);
> 	*cookie = retbuf[0];
> 	*irq = retbuf[1];
>
> 	return rc;
> }
>
> In plpar_hcall() (arch/powerpc/platforms/pseries/hvCall.S), r5 (unit_address)
> is moved into r4 before HVSC and r4 is unconditionally stored into retbuf[0]
> after HVSC, even when HVSC returns an error such as H_PARAMETER or H_RESOURCE.
>
> When h_reg_sub_crq() fails in ibmvfc_register_channel():
>
> 	rc = h_reg_sub_crq(vdev->unit_address, scrq->msg_token, PAGE_SIZE,
> 			   &scrq->cookie, &scrq->hw_irq);
>
> 	/* H_CLOSED indicates successful register, but no CRQ partner */
> 	if (rc && rc != H_CLOSED) {
> 		dev_warn(dev, "Error registering sub-crq: %d\n", rc);
> 		if (rc == H_PARAMETER)
> 			dev_warn_once(dev, "Firmware may not support MQ\n");
> 		goto reg_failed;
> 	}
>
> jumping to reg_failed skips scrq->cookie = 0, leaving a non-zero value in
> scrq->cookie. If ibmvfc_deregister_channel() is later called on this queue
> during reset or teardown (via ibmvfc_dereg_sub_crqs()), won't the new
> if (scrq->cookie) check evaluate to true and issue H_FREE_SUB_CRQ on a queue
> that failed registration?

Yes. I will move clearing scrq->cookie after the reg_failed label.

-Dave