Re: [PATCH v2 08/23] dmaengine: sdxi: Install administrative context

[Nathan Lynch <nathan.lynch@amd.com>]

sashiko-bot@kernel.org writes:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Hardware Use-After-Free due to missing admin context teardown
> - [High] Torn writes on 32-bit platforms exposing incomplete addresses to hardware
> - [Medium] Incorrect error check for `dma_pool_zalloc()` using the DMA handle instead of the virtual pointer
> --
>
> commit 794d6cb84f7ad6a7699311744a5e9c87b9d20316
> Author: Nathan Lynch <nathan.lynch@amd.com>
>
> dmaengine: sdxi: Install administrative context
>
> This commit serializes the context control block, akey table, and L1 entry for
> the admin context. It makes the descriptor ring, write index, and context
> status block visible to the SDXI implementation once activated.
>
>> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
>> index 27821cfaf0311..c0b55c945cc4e 100644
>> --- a/drivers/dma/sdxi/context.c
>> +++ b/drivers/dma/sdxi/context.c
>
> [ ... ]
>
>> @@ -106,6 +112,152 @@ static struct sdxi_cxt *sdxi_alloc_cxt(struct sdxi_dev *sdxi)
>
> [ ... ]
>
>> +static int configure_cxt_ctl(struct sdxi_cxt_ctl *ctl, const struct sdxi_cxt_ctl_cfg *cfg)
>> +{
>> +	u64 ds_ring_ptr, cxt_sts_ptr, write_index_ptr;
>
> [ ... ]
>
>> +	ds_ring_ptr = FIELD_PREP(SDXI_CXT_CTL_VL, 1) |
>> +		      FIELD_PREP(SDXI_CXT_CTL_QOS, cfg->qos) |
>> +		      FIELD_PREP(SDXI_CXT_CTL_SE, cfg->se) |
>> +		      FIELD_PREP(SDXI_CXT_CTL_CSA, cfg->csa) |
>> +		      FIELD_PREP(SDXI_CXT_CTL_DS_RING_PTR,
>> +				 cfg->ds_ring_ptr >> DESC_RING_BASE_PTR_SHIFT);
>> +	/* Ensure other fields are visible before hw sees vl=1. */
>> +	dma_wmb();
>> +	WRITE_ONCE(ctl->ds_ring_ptr, cpu_to_le64(ds_ring_ptr));
>
> Can this introduce a regression with torn writes on 32-bit platforms?

As mentioned elsewhere the driver will just depend on CONFIG_64BIT for
now.


>> +static void free_admin_cxt(void *ptr)
>> +{
>> +	struct sdxi_dev *sdxi = ptr;
>> +
>> +	sdxi_free_cxt(sdxi->admin_cxt);
>> +}
>
> Does this sequence cause a use-after-free regression?
>
> In sdxi_publish_cxt(), the admin context is published to the hardware by
> setting the valid bit (VL=1) in the L1_table.
>
> When the context is torn down via the devres cleanup action free_admin_cxt(),
> it directly invokes sdxi_free_cxt(), which frees the DMA memory for the
> context control block, descriptor ring, and akey table.
>
> However, the VL bit in the hardware's L1 table entry is never cleared. Could
> this leave the hardware actively fetching from freed physical
> addresses?

No I don't think so, the code as written ensures the device is in
stopped state when we get to the point of freeing the admin context
control structures.


> Additionally, looking at the sdxi_alloc_cxt() implementation, the return value
> of dma_pool_zalloc() is checked via the DMA address instead of the virtual
> pointer:
>
> 	cxt->cxt_ctl = dma_pool_zalloc(sdxi->cxt_ctl_pool, GFP_KERNEL,
> 				       &cxt->cxt_ctl_dma);
> 	if (!cxt->cxt_ctl_dma)
> 		return NULL;
>
> Since physical address 0 is a valid DMA address on many architectures and IOMMU
> configurations, could this regression incorrectly abort initialization if a
> successful allocation returns DMA address 0? Should this code check the
> returned virtual pointer (!cxt->cxt_ctl) instead?

Valid bug but I think this is a redundant report, the bot pointed it out
elsewhere in the thread.