Re: [PATCH v6 09/14] gtrace: Add RISC-V Trace ramsink driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gtrace: DMA use-after-free on driver unbind while tracing
- [Medium] gtrace: out-of-bounds read if hardware write pointer exceeds limit
--

commit 727d829afd136c7c3871f88297ecd560269b3dbd
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

gtrace: Add RISC-V Trace ramsink driver

This commit adds the initial implementation of the RISC-V E-Trace ramsink
driver. The ramsink stores trace data generated by upstream components into
a system memory buffer.

> diff --git a/drivers/hwtracing/gtrace/rvtrace-ramsink.c b/drivers/hwtracing/gtrace/rvtrace-ramsink.c
> new file mode 100644
> index 0000000000000..67b6779aaceec
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/rvtrace-ramsink.c

[ ... ]

> +static size_t rvtrace_ramsink_copyto_auxbuf(struct gtrace_component *comp,
> +					    struct gtrace_perf_auxbuf *buf,
> +					    u64 *format)
> +{
> +	struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
> +	struct trace_buf src, dst;
> +	size_t bytes = 0;
> +	bool wrap;
> +	u64 wp;
> +
> +	dst.base = buf->base;
> +	dst.len = buf->length;
> +	dst.cur = buf->pos;
> +	src.base = priv->va;
> +	src.len = priv->size;
> +	*format = PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE;
> +	wp = rvtrace_ramsink_read_pair(comp->pdata, RVTRACE_RAMSINK_WPLOW_OFF);
> +	wrap = wp & RVTRACE_RAMSINK_WPLOW_WRAP;
> +	wp &= ~RVTRACE_RAMSINK_WPLOW_WRAP;
> +
> +	if (wrap) {
> +		rvtrace_ramsink_write_pair(comp->pdata, priv->start, RVTRACE_RAMSINK_WPLOW_OFF);
> +		src.cur = wp - priv->start;
> +		priv->prev_wp = priv->start;
> +		/*
> +		 * There is no way to tell if trRamWp wrapped around more than once. As a
> +		 * result priv->prev_wp can't be used and the entire buffer must be copied
> +		 * even though some data might be duplicated.
> +		 */
> +		bytes = priv->size;
> +	} else {
> +		src.cur =  priv->prev_wp - priv->start;
> +		bytes = wp - priv->prev_wp;
> +		priv->prev_wp = wp;
> +	}

[Severity: Medium]
Does this code safely handle anomalous hardware behavior? If the hardware
write pointer (wp) read directly from RVTRACE_RAMSINK_WPLOW_OFF exceeds the
configured priv->end limit, it seems src.cur could underflow or exceed the
buffer length (priv->size).

Without bounds checking on the offset returned by hardware, could this
cause the subsequent memcpy in tbuf_to_pbuf_copy to read out-of-bounds
kernel memory and leak it into the userspace perf ring buffer?

> +
> +	tbuf_to_pbuf_copy(&src, &dst, bytes);
> +	dev_dbg(&comp->dev, "Copied %zu bytes\n", bytes);
> +	return bytes;
> +}

[ ... ]

> +static void rvtrace_ramsink_cleanup(struct gtrace_component *comp)
> +{
> +	struct rvtrace_ramsink_priv *priv = dev_get_drvdata(&comp->dev);
> +
> +	dma_free_coherent(comp->pdata->dev, priv->alloc_size, priv->alloc_va, priv->alloc_start);
> +}

[ ... ]

> +static void rvtrace_ramsink_remove(struct gtrace_component *comp)
> +{
> +	rvtrace_ramsink_cleanup(comp);
> +}

[Severity: High]
Can unbinding this driver while tracing is active lead to a DMA
use-after-free?

rvtrace_ramsink_remove calls rvtrace_ramsink_cleanup which immediately
frees the DMA memory via dma_free_coherent. If the hardware isn't
explicitly halted via gtrace_disable_component first, the hardware might
continue writing trace data to the freed memory pages.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001052243.3042627-1-mayuresh.chitale@oss.qualcomm.com?part=9