Re: [PATCH] NTB: ntb_transport: fix use after free

[Baruch Siach <baruch@tkos.co.il>]

Hi Koichiro,

On Thu, Nov 06 2025, Koichiro Den wrote:
> On Tue, Nov 04, 2025 at 07:42:53AM +0200, Baruch Siach wrote:
>> Thanks for taking the time to review this.
>> 
>> On Tue, Nov 04 2025, Koichiro Den wrote:
>> > On Mon, Nov 03, 2025 at 07:23:19PM +0200, Baruch Siach wrote:
>> >> Don't call dmaengine_unmap_put() twice for the same pointer. This
>> >> results in mempool_free() being called on a freed element.
>> >
>> > While the second dmaengine_unmap_put() looks redundant at first glance, my
>> > understanding was that it compensates for the refcount decrement that would
>> > normally happen on the dmaengine completion path, which never occurs when
>> > dmaengine_submit() fails. Am I missing something?
>> 
>> I guess you are right. Now I see that dma_set_unmap() increments the
>> refcount. But I can't find the second refcount decrement in completion
>> path. I only see one call to dmaengine_unmap_put(). Can you enlighten
>> me?
>
> Apologies for the late response. The dma_descriptor_unmap() wrapper should
> handle that, shouldn't it?

Probably. The dmaengine driver I'm using is dw-axi-dmac, and I see no
call to dma_descriptor_unmap() there. Not sure where the bug is, but
it's definitely not in the NTB scope.

Thanks for your help. Sorry for the noise.

baruch

>> >> Fixes: 6f57fd0578df ("NTB: convert to dmaengine_unmap_data")
>> >> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> >> ---
>> >>  drivers/ntb/ntb_transport.c | 22 +++++++++-------------
>> >>  1 file changed, 9 insertions(+), 13 deletions(-)
>> >> 
>> >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
>> >> index eb875e3db2e3..809fb09658b4 100644
>> >> --- a/drivers/ntb/ntb_transport.c
>> >> +++ b/drivers/ntb/ntb_transport.c
>> >> @@ -1573,14 +1573,14 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset),
>> >>  				      pay_off, len, DMA_TO_DEVICE);
>> >>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	unmap->to_cnt = 1;
>> >>  
>> >>  	unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf),
>> >>  				      buff_off, len, DMA_FROM_DEVICE);
>> >>  	if (dma_mapping_error(device->dev, unmap->addr[1]))
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	unmap->from_cnt = 1;
>> >>  
>> >> @@ -1588,7 +1588,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  					     unmap->addr[0], len,
>> >>  					     DMA_PREP_INTERRUPT);
>> >>  	if (!txd)
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	txd->callback_result = ntb_rx_copy_callback;
>> >>  	txd->callback_param = entry;
>> >> @@ -1596,7 +1596,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  
>> >>  	cookie = dmaengine_submit(txd);
>> >>  	if (dma_submit_error(cookie))
>> >> -		goto err_set_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	dmaengine_unmap_put(unmap);
>> >>  
>> >> @@ -1606,9 +1606,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>> >>  
>> >>  	return 0;
>> >>  
>> >> -err_set_unmap:
>> >> -	dmaengine_unmap_put(unmap);
>> >> -err_get_unmap:
>> >> +err_unmap:
>> >>  	dmaengine_unmap_put(unmap);
>> >>  err:
>> >>  	return -ENXIO;
>> >> @@ -1854,14 +1852,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>> >>  	unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf),
>> >>  				      buff_off, len, DMA_TO_DEVICE);
>> >>  	if (dma_mapping_error(device->dev, unmap->addr[0]))
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	unmap->to_cnt = 1;
>> >>  
>> >>  	txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len,
>> >>  					     DMA_PREP_INTERRUPT);
>> >>  	if (!txd)
>> >> -		goto err_get_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	txd->callback_result = ntb_tx_copy_callback;
>> >>  	txd->callback_param = entry;
>> >> @@ -1869,16 +1867,14 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>> >>  
>> >>  	cookie = dmaengine_submit(txd);
>> >>  	if (dma_submit_error(cookie))
>> >> -		goto err_set_unmap;
>> >> +		goto err_unmap;
>> >>  
>> >>  	dmaengine_unmap_put(unmap);
>> >>  
>> >>  	dma_async_issue_pending(chan);
>> >>  
>> >>  	return 0;
>> >> -err_set_unmap:
>> >> -	dmaengine_unmap_put(unmap);
>> >> -err_get_unmap:
>> >> +err_unmap:
>> >>  	dmaengine_unmap_put(unmap);
>> >>  err:
>> >>  	return -ENXIO;
>> >> -- 
>> >> 2.51.0
>> >> 
>> 
>> -- 
>>                                                      ~. .~   Tk Open Systems
>> =}------------------------------------------------ooO--U--Ooo------------{=
>>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -