Re: [PATCH v18 05/13] scsi: ibmvfc: fix resource leaks on partial queue allocation failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ibmvfc: stale evt_pool state in ibmvfc_free_queue() on partial alloc
--

commit 226240ace51c2767f2bb52b147409116cfbb8d78
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: fix resource leaks on partial queue allocation failure

This commit guards dma_unmap_single() and free_page() in ibmvfc_free_queue()
under a queue->msgs.handle check and adds ibmvfc_free_event_pool() calls on
the error paths of ibmvfc_alloc_queue() after ibmvfc_init_event_pool()
succeeds.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 77c685f6f959b..e04c80050b981 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -914,9 +914,11 @@ static void ibmvfc_free_queue(struct ibmvfc_host *vhost,
>  {
>  	struct device *dev = vhost->dev;
>  
> -	dma_unmap_single(dev, queue->msg_token, PAGE_SIZE, DMA_BIDIRECTIONAL);
> -	free_page((unsigned long)queue->msgs.handle);
> -	queue->msgs.handle = NULL;
> +	if (queue->msgs.handle) {
> +		dma_unmap_single(dev, queue->msg_token, PAGE_SIZE, DMA_BIDIRECTIONAL);
> +		free_page((unsigned long)queue->msgs.handle);
> +		queue->msgs.handle = NULL;
> +	}
>  
>  	ibmvfc_free_event_pool(vhost, queue);
>  }
> @@ -6303,8 +6305,10 @@ static int ibmvfc_alloc_queue(struct ibmvfc_host *vhost,
>  	}
>  
>  	queue->msgs.handle = (void *)get_zeroed_page(GFP_KERNEL);
> -	if (!queue->msgs.handle)
> +	if (!queue->msgs.handle) {
> +		ibmvfc_free_event_pool(vhost, queue);
>  		return -ENOMEM;
> +	}
>  
>  	queue->msg_token = dma_map_single(dev, queue->msgs.handle, PAGE_SIZE,
>  					  DMA_BIDIRECTIONAL);
> @@ -6312,6 +6316,7 @@ static int ibmvfc_alloc_queue(struct ibmvfc_host *vhost,
>  	if (dma_mapping_error(dev, queue->msg_token)) {
>  		free_page((unsigned long)queue->msgs.handle);
>  		queue->msgs.handle = NULL;
> +		ibmvfc_free_event_pool(vhost, queue);
>  		return -ENOMEM;
>  	}

[Severity: High]
The commit message states that guarding dma_unmap_single() and free_page()
under queue->msgs.handle makes ibmvfc_free_queue() safe to call on a
partially initialized queue. However, does calling ibmvfc_free_queue() after
a partial queue allocation failure still trigger a use-after-free,
double-free, or NULL pointer dereference in ibmvfc_free_event_pool()?

In drivers/scsi/ibmvscsi/ibmvfc-core.c, ibmvfc_init_event_pool() sets
pool->size before allocating pool->events and pool->iu_storage, and does not
reset pool->size or clear pool->events on failure:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_init_event_pool() {
	...
	pool->size = queue->total_depth;
	pool->events = kzalloc_objs(*pool->events, pool->size);
	if (!pool->events)
		return -ENOMEM;

	pool->iu_storage = dma_alloc_coherent(vhost->dev,
					      pool->size * sizeof(*pool->iu_storage),
					      &pool->iu_token, 0);

	if (!pool->iu_storage) {
		kfree(pool->events);
		return -ENOMEM;
	}
	...
}

Similarly, when ibmvfc_alloc_queue() fails at get_zeroed_page() or
dma_mapping_error() and calls ibmvfc_free_event_pool(), pool->events and
pool->iu_storage are freed, but pool->size, pool->events, and
pool->iu_storage are left with their old values:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_free_event_pool() {
	...
	for (i = 0; i < pool->size; ++i) {
		list_del(&pool->events[i].queue_list);
		BUG_ON(atomic_read(&pool->events[i].free) != 1);
		if (pool->events[i].ext_list)
			dma_pool_free(vhost->sg_pool,
				      pool->events[i].ext_list,
				      pool->events[i].ext_list_token);
	}

	kfree(pool->events);
	dma_free_coherent(vhost->dev,
			  pool->size * sizeof(*pool->iu_storage),
			  pool->iu_storage, pool->iu_token);
	LEAVE;
}

Because ibmvfc_free_queue() still calls ibmvfc_free_event_pool()
unconditionally outside the if (queue->msgs.handle) check, calling
ibmvfc_free_queue() on a queue after ibmvfc_alloc_queue() fails (for
IBMVFC_CRQ_FMT or IBMVFC_SUB_CRQ_FMT where queue->total_depth is non-zero)
will iterate over the stale pool->size and either dereference a NULL
pool->events pointer or access and free pool->events and pool->iu_storage a
second time.

Should ibmvfc_free_event_pool() also be guarded or reset pool->size,
pool->events, and pool->iu_storage so that ibmvfc_free_queue() is safe on a
partially initialized queue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-ibmvfc-fpin-support-v18-0-d683b74736a3@linux.ibm.com?part=5