Re: [PATCH 1/3] drm/bridge: nwl-dsi: Limit LCDIF specific sync override

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Dangling pointer `dsi->xfer` leads to stack Use-After-Free if a timeout or clock error occurs during transfer.
> --
>
> --- Patch [1]: [PATCH 1/3] drm/bridge: nwl-dsi: Limit LCDIF specific sync override ---
> commit 36a9bfa60fd8b6568d59af4e95e04fc059970758
> Author: Esben Haabendal <esben@geanix.com>
>
> drm/bridge: nwl-dsi: Limit LCDIF specific sync override
>
> When using DCSS with NWL, overriding the mode flags to enforce active high
> sync is preventing the use of active low with downstream bridges, such as
> ti-sn65dsi83, which will not see such mode flags set by the panel.
>
>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
>> index 3c9ae93c4f671..915e1bdc73c80 100644
>> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
>> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
>> @@ -110,6 +110,8 @@ struct nwl_dsi {
>>  	int error;
>>
>>  	struct nwl_dsi_transfer *xfer;
>
> [Severity: High]
> This is a pre-existing issue, but could this pointer lead to a stack
> use-after-free if a timeout or clock error occurs during a transfer?
>
> In nwl_dsi_host_transfer(), dsi->xfer is assigned the address of a local stack
> variable:
>
> 	struct nwl_dsi_transfer xfer;
> 	...
> 	dsi->xfer = &xfer;
>
> If clk_prepare_enable(dsi->rx_esc_clk) fails, or if the DSI transfer times out
> (wait_for_completion_timeout() returns 0), the function returns without
> clearing dsi->xfer.
>
> If a delayed or spurious hardware interrupt later triggers
> nwl_dsi_irq_handler(), it will call nwl_dsi_finish_transmission() which
> dereferences the dangling dsi->xfer pointer:
>
> 	xfer->status = xfer->tx_len;
> 	complete(&xfer->completed);
>
> Because xfer was originally on the stack of the returned function, could this
> overwrite arbitrary stack memory of whatever thread happens to be running at
> the time of the interrupt?

That does sound like a problem, yes. But unrelated to the change under
review here.

Perhaps dsi->xfer should be changed into a an embedded struct
nwl_dsi_transfer member instead of a pointer (to stack)? Although I fear
that the same kind of situation will then trigger another problem, as a
nwl_dsi_host_transfer() would then create a new packet while
nwl_dsi_finish_transmission() could be using the packet as well.


>> +
>> +	unsigned int endpoint;
>>  };

/Esben