Re: [PATCH v13 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 2 potential issue(s) to consider:
> - [High] scsi: ibmvfc: double free of sub-CRQs on partial registration failure
> - [Low] scsi: ibmvfc: unprotected H_FREE_SUB_CRQ on uninitialized or double-freed queues
> --
>
> commit 0fde5959dd5d7f8887aa67ee2fa7032b0dd6353a
> Author: Tyrel Datwyler <tyreld@linux.ibm.com>
>
> scsi: ibmvfc: fix potential NULL pointer dereference on failed queue allocation
>
> Guard the memset() in ibmvfc_deregister_channel() behind a check of
> scrq->msgs.handle to prevent a NULL dereference when the function is
> called on a queue that was never allocated, such as async_sub_crq when
> multi-queue is disabled or initialization failed before
> ibmvfc_alloc_queue() was reached.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 145d559041f12..6a42d15db9ec5 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> @@ -6480,8 +6480,10 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>>  		dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n", index, rc);
>
> [Severity: High]
> Can a partial registration failure lead to a double-free of the sub-CRQ
> resource?
>
> In ibmvfc_reg_sub_crqs(), if registration fails midway, it deregisters the
> already registered queues by calling ibmvfc_deregister_channel():
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_reg_sub_crqs() {
>     ...
> 	for (i = 0; i < channels->max_queues; i++) {
> 		if (ibmvfc_register_channel(vhost, channels, i)) {
> 			for (j = i; j > 0; j--)
> 				ibmvfc_deregister_channel(vhost, channels, j - 1);
>     ...
> }
>
> However, ibmvfc_deregister_channel() doesn't zero scrq->cookie, and
> vhost->mq_enabled remains true. 
>
> When a subsequent adapter reset or teardown occurs, ibmvfc_dereg_sub_crqs()
> loops over all queues and calls ibmvfc_deregister_channel() again on the
> same queues, potentially causing a double-free at the hypervisor level.
>
> Could scrq->cookie be cleared after successfully freeing the sub-CRQ to
> prevent this?
>
>>  	/* Clean out the queue */
>> -	memset(scrq->msgs.crq, 0, PAGE_SIZE);
>> -	scrq->cur = 0;
>> +	if (scrq->msgs.handle) {
>
> [Severity: Low]
> Is the H_FREE_SUB_CRQ hypercall safe to execute on uninitialized queues?
>
> The memset() is now properly guarded by scrq->msgs.handle, but the
> H_FREE_SUB_CRQ hypercall just above this block continues to run
> unconditionally.
>
> If a queue was never registered or was already deregistered (such as during
> the ibmvfc_reg_sub_crqs() rollback), the unconditional hypercall will fail
> and generate a spurious "Failed to free sub-crq..." error message.
>
> Should the hypercall logic also be protected against uninitialized or already
> freed queues?

Both of these issues will be resolve by check scrq.cookie. In the "never
registered" case, cookie is still 0 from the initial allocation by
kzalloc_obj(). Clearing scrq.cookie on deregistration will be added in
ibmvfc_deregister_channel(), and calling the hypervisor will also be
conditional on scrq.cookie != 0.

-Dave