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

[sashiko-bot@kernel.org]

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?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-ibmvfc-fpin-support-v15-0-a858726e178a@linux.ibm.com?part=3