Re: [PATCH net v2] dibs: fix double free of dmb_clientid_arr

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] dibs_unregister_client() fails with partial unregistration if an active DMB is found on a device later in the list, leaving the client in a corrupted state.
- [High] Initialization failure in dibs_init() leaks dibs_class and causes Use-After-Free.
- [High] Early registration of ism_handle_irq() in ism_probe() causes potential NULL Pointer Dereference and Use-After-Free.
- [High] `dibs_lo_move_data()` acquires `dibs->lock` with plain `spin_lock()`, missing IRQ masking.
- [High] In the error path of `ism_alloc_dmb()`, a folio allocated page is illegally freed using `kfree()`.
--

--- Patch [1]: [PATCH net v2] dibs: fix double free of dmb_clientid_arr ---
commit c4dab923b3875e2fdb8522fd524541f2cd53cf25
Author: Hidayath Khan <[email protected]>
Subject: dibs: fix double free of dmb_clientid_arr

This commit removes the kfree() call in dibs_lo_dev_probe() to prevent a
double free of dibs->dmb_clientid_arr. The pointer is already freed on both
failure paths of dibs_dev_add(), which is called just before the error path.

> diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
> index fd5caf1e19a8f..649e4e375be33 100644
> --- a/drivers/dibs/dibs_loopback.c
> +++ b/drivers/dibs/dibs_loopback.c
> @@ -335,7 +335,6 @@ static int dibs_lo_dev_probe(void)
>  	return 0;
>  
>  err_reg:
> -	kfree(dibs->dmb_clientid_arr);
>  	/* pairs with dibs_dev_alloc() */
>  	put_device(&dibs->dev);
>  	kfree(ldev);

While reviewing this area, I noticed a few pre-existing issues that were not
introduced by this patch.

drivers/dibs/dibs_main.c:dibs_unregister_client()
> 		for (int i = 0; i < max_dmbs; ++i) {
> 			if (dibs->dmb_clientid_arr[i] == client->id) {
> 				WARN(1, "%s: attempt to unregister '%s' with registered dmb(s)\n",
> 				     __func__, client->name);
> 				rc = -EBUSY;
> 				goto err_reg_dmb;
> 			}
> 		}
> 		/* Stop forwarding IRQs and events */
> 		dibs->subs[client->id] = NULL;
> 		spin_unlock_irqrestore(&dibs->lock, flags);
> 		clients[client->id]->ops->del_dev(dibs);

[Severity: High]
This isn't a bug introduced by this patch, but if dibs_unregister_client()
finds an active DMB on a device later in the list, it aborts and returns
-EBUSY. Does this leave the client in a corrupted state since the earlier
devices in the list already had their forwarding stopped and del_dev() called?
A subsequent retry could result in a double-free or use-after-free when
del_dev() is called again for the already processed devices.

drivers/dibs/dibs_main.c:dibs_init()
> static int __init dibs_init(void)
> {
> 	int rc;
> 
> 	rc = class_register(&dibs_class);
> 	if (rc)
> 		return rc;
> 
> 	rc = dibs_loopback_init();
> 	if (rc)
> 		pr_err("%s fails with %d\n", __func__, rc);
> 
> 	return rc;
> }

[Severity: High]
This is a pre-existing issue, but does an initialization failure here leak the
dibs_class? If dibs_loopback_init() fails, the module load is aborted and
memory is freed, but class_register() isn't undone. Could this result in a
use-after-free if sysfs accesses the freed module memory?

drivers/s390/net/ism_drv.c:ism_probe()
> 	/* enable ism device, but any interrupts and events will be ignored
> 	 * before dibs_dev_add() adds it to any clients.
> 	 */
> 	ret = ism_dev_init(ism);
> 	if (ret)
> 		goto err_dibs;
> ...
> 	ret = dibs_dev_add(dibs);

drivers/s390/net/ism_drv.c:ism_handle_irq()
> 	spin_lock(&dibs->lock);
> 	ism->sba->s = 0;
> 	barrier();
> ...
> 		client_id = dibs->dmb_clientid_arr[bit];

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for an early
interrupt to trigger a NULL pointer dereference or use-after-free?
ism_dev_init() enables the hardware interrupts before dibs_dev_add()
initializes dibs->lock and allocates dibs->dmb_clientid_arr. If an interrupt
fires early, could ism_handle_irq() attempt to acquire an uninitialized lock
and dereference a NULL pointer?

drivers/dibs/dibs_loopback.c:dibs_lo_move_data()
> 	spin_lock(&dibs->lock);
> 	client_id = dibs->dmb_clientid_arr[sba_idx];
> 	s_mask = ror16(0x1000, idx);
> 	if (likely(client_id != NO_DIBS_CLIENT && dibs->subs[client_id]))
> 		dibs->subs[client_id]->ops->handle_irq(dibs, sba_idx, s_mask);
> 	spin_unlock(&dibs->lock);

[Severity: High]
This is a pre-existing issue, but is it safe to acquire dibs->lock using
spin_lock() here? Since the lock is also acquired in the hardirq context by
ism_handle_irq(), could acquiring it here without disabling interrupts cause a
deadlock if an interrupt arrives on the same CPU?

drivers/s390/net/ism_drv.c:ism_alloc_dmb()
> 	folio = folio_alloc(GFP_KERNEL | __GFP_NOWARN | __GFP_NOMEMALLOC |
> 			    __GFP_NORETRY, get_order(dmb->dmb_len));
> 
> 	if (!folio) {
> 		rc = -ENOMEM;
> 		goto out_bit;
> 	}
> 
> 	dmb->cpu_addr = folio_address(folio);
> 	dmb->dma_addr = dma_map_page(&ism->pdev->dev,
> 				     virt_to_page(dmb->cpu_addr), 0,
> 				     dmb->dmb_len, DMA_FROM_DEVICE);
> 	if (dma_mapping_error(&ism->pdev->dev, dmb->dma_addr)) {
> 		rc = -ENOMEM;
> 		goto out_free;
> 	}
> 
> 	return 0;
> 
> out_free:
> 	kfree(dmb->cpu_addr);

[Severity: High]
This isn't a bug introduced by this patch, but does the error path incorrectly
free the page memory? Since dmb->cpu_addr was allocated via folio_alloc(),
shouldn't it be freed using folio_put() instead of kfree() to avoid slab
allocator corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1