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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Modifying DMA pointers and size after allocation causes a DMA API violation upon freeing and out-of-bounds memory accesses.
- [High] Missing bounds check on the hardware write pointer (`wp`) allows arbitrary out-of-bounds kernel memory reads.
- [High] Trusting hardware limit registers unconditionally overwrites `priv->size`, allowing hardware to inflate buffer bounds.
- [High] Trusting hardware limit registers can result in a zero-sized buffer, triggering a division-by-zero panic.
- [High] Using signed `int` for lengths in `tbuf_to_pbuf_copy` causes an integer overflow when bounds exceed 2GB.
- [Medium] Bitwise OR operations update configuration fields without clearing previous bit values, leaving old configurations active.
- [Medium] The driver does not export its component ID table, preventing automatic module loading.
- [Medium] Buffer truncation logic error when hardware clamps the limit address.
- [Medium] Missing synchronization protecting shared ramsink state in `copyto_auxbuf`.
- [Low] Format String Type Mismatch using `%pad` for `u64` variables.
--

--- Patch [9]: [PATCH v5 09/14] gtrace: Add RISC-V Trace ramsink driver ---
commit 07a5e011c3c5f12851ee9400f1bd383cb0538970
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

gtrace: Add RISC-V Trace ramsink driver

This patch adds the initial implementation of the RISC-V E-Trace ramsink
driver. The ramsink stores trace data into a system memory buffer for perf
AUX buffer usage.

> diff --git a/drivers/hwtracing/gtrace/rvtrace-ramsink.c b/drivers/hwtracing/gtrace/rvtrace-ramsink.c
> new file mode 100644
> index 0000000000000..7ed25cf364a71
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/rvtrace-ramsink.c
[ ... ]
> +static void tbuf_to_pbuf_copy(struct trace_buf *src, struct trace_buf *dst, size_t size)
> +{
> +	int bytes_dst, bytes_src, bytes;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code risk integer overflow if the lengths exceed 2GB?

The variables bytes, bytes_dst, and bytes_src are declared as signed int,
while the lengths they represent (like dst->len and src->len) are size_t
and can legally exceed 2GB. If dst->len - dst->cur exceeds 2GB, the
subtraction overflows the 32-bit int into a negative number.

Could this negative number pass the bytes_dst < bytes_src check and be
supplied to memcpy(), which implicitly casts it to a huge size_t value
causing a fatal out-of-bounds copy?

> +	void *dst_addr, *src_addr;
> +
> +	/* If destination cannot hold entire source buffer then write only the latest data. */
> +	if (dst->len < size) {
> +		src->cur = (src->cur + size - dst->len) % src->len;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Could this result in a division-by-zero panic?

If the hardware limit register readback exactly equals priv->start,
rvtrace_ramsink_setup_buf() will update priv->end = priv->start, making
priv->size = 0.

Upon the next perf AUX buffer read, rvtrace_ramsink_copyto_auxbuf() will pass
priv->size (0) as src.len to tbuf_to_pbuf_copy(). This would cause the modulo
operation here to divide by zero.

> +		size = dst->len;
> +	}
> +
> +	while (size) {
[ ... ]
> +		if (src->len - src->cur < size)
> +			bytes_src = src->len - src->cur;
> +		else
> +			bytes_src = size;
> +		bytes = bytes_dst < bytes_src ? bytes_dst : bytes_src;
> +		memcpy(dst_addr, src_addr, bytes);
> +		dst->cur = (dst->cur + bytes) % dst->len;
> +		src->cur = (src->cur + bytes) % src->len;
> +		size -= bytes;
> +	}
> +}
> +
> +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;
> +	u32 wp_low, wp_high;
> +	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;
> +	wp_low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPLOW_OFF);
> +	wp_high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_WPHIGH_OFF);
> +	wp = (u64)(wp_high) << 32 | wp_low;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this implicitly trust the hardware read value without bounds validation?

If a faulty hardware component returns a value outside the physical buffer
bounds, or if concurrent accesses cause a race, the calculations like
bytes = wp - priv->prev_wp could underflow or overflow.

This would allow tbuf_to_pbuf_copy() to copy arbitrary out-of-bounds kernel
memory directly into the perf AUX buffer, leaking kernel memory to userspace.

> +	wrap = wp & RVTRACE_RAMSINK_WPLOW_WRAP;
> +	wp &= ~RVTRACE_RAMSINK_WPLOW_WRAP;
> +	if (wrap) {
[ ... ]
> +	} else {
> +		src.cur =  priv->prev_wp - priv->start;
> +		bytes = wp - priv->prev_wp;
> +		priv->prev_wp = wp;
                ^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Is there missing synchronization protecting this shared ramsink state?

If rvtrace_ramsink_copyto_auxbuf() is executed concurrently, data races
on priv->prev_wp could result in length underflows and corrupted hardware
write pointers. Is a spinlock necessary to protect this component's
shared state?

> +	}
> +
> +	tbuf_to_pbuf_copy(&src, &dst, bytes);
> +	dev_dbg(&comp->dev, "Copied %zu bytes\n", bytes);
> +	*format = PERF_AUX_FLAG_GTRACE_FORMAT_ETRACE;
> +	return bytes;
> +}
> +
> +static int rvtrace_ramsink_setup_buf(struct gtrace_component *comp,
> +				     struct rvtrace_ramsink_priv *priv)
> +{
> +	struct device *pdev = comp->pdata->dev;
> +	u64 start_min, limit_max, end;
> +	u32 low, high;
> +	int ret;
[ ... ]
> +	priv->va = dma_alloc_coherent(pdev, priv->size, &priv->start, GFP_KERNEL);
> +	if (!priv->va)
> +		return -ENOMEM;
> +
> +	priv->end = priv->start + priv->size;
> +	priv->prev_wp = priv->start;
> +	if (priv->end <= start_min || priv->start >= limit_max) {
> +		dma_free_coherent(pdev, priv->size, priv->va, priv->start);
> +		dev_err(&comp->dev, "DMA memory not addressable by device\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Setup ram sink start addresses */
> +	if (priv->start < start_min) {
> +		dev_warn(&comp->dev, "Ramsink start address updated from %pad to %pad\n",
> +			 &priv->start, &start_min);
                                       ^^^^^^^^^^

[Severity: Low]
Does this code use an incorrect format string for start_min?

start_min is of type u64, but %pad requires a pointer to dma_addr_t or
phys_addr_t. On 32-bit architectures where dma_addr_t is 32-bit, printk
will dereference the pointer as a 32-bit type, reading only half of the
u64 variable, resulting in incorrect log output.

> +		priv->va += start_min - priv->start;
> +		priv->start = start_min;
                ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can modifying these DMA pointers cause a DMA API violation later?

When rvtrace_ramsink_cleanup() later passes these modified pointers to
dma_free_coherent(), it violates the DMA API, which strictly requires the
exact virtual and physical addresses originally returned by
dma_alloc_coherent().

This can lead to a kernel panic on driver unbind. Also, because priv->size
is not reduced, operations accessing priv->start + priv->size will overflow
the allocated DMA buffer bounds.

> +	}
> +
> +	gtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_STARTLOW_OFF);
> +	gtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_STARTHIGH_OFF);
> +	gtrace_write32(comp->pdata, lower_32_bits(priv->start), RVTRACE_RAMSINK_WPLOW_OFF);
> +	gtrace_write32(comp->pdata, upper_32_bits(priv->start), RVTRACE_RAMSINK_WPHIGH_OFF);
> +	/* Setup ram sink limit addresses */
> +	if (priv->end > limit_max) {
> +		dev_warn(&comp->dev, "Ramsink limit address updated from %pad to %pad\n",
> +			 &priv->end, &limit_max);
> +		priv->end = limit_max;
> +		priv->size = priv->end - priv->start;
> +	}
> +
> +	/* Limit address needs to be set to end - mem_access_width to avoid overflow */
> +	end = priv->end - priv->mem_acc_width;
> +	gtrace_write32(comp->pdata, lower_32_bits(end), RVTRACE_RAMSINK_LIMITLOW_OFF);
> +	gtrace_write32(comp->pdata, upper_32_bits(end), RVTRACE_RAMSINK_LIMITHIGH_OFF);
> +	low = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITLOW_OFF);
> +	high = gtrace_read32(comp->pdata, RVTRACE_RAMSINK_LIMITHIGH_OFF);
> +	end = (u64)(high) << 32 | low;
> +	if (end != (priv->end - 4)) {
> +		dev_warn(&comp->dev, "Ramsink limit address updated from %pad to %pad\n",
> +			 &priv->end, &end);
                                     ^^^^

[Severity: Low]
Does this code use an incorrect format string for end?

end is of type u64, but %pad requires a pointer to dma_addr_t or
phys_addr_t.

> +		priv->end = end;
> +		priv->size = priv->end - priv->start;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this trust the hardware limit registers to unconditionally overwrite
priv->size?

If the hardware returns a value much larger than the original 1MB DMA
allocation, the software size limits are inflated. Subsequent trace data
copies will read past the 1MB physical buffer, leaking surrounding kernel
memory into the user perf stream.

[Severity: Medium]
Additionally, does this introduce a buffer truncation logic error?

The driver subtracted 4 (priv->mem_acc_width) to avoid hardware wrap
overflow. If the hardware clamps this limit and returns a different value,
shrinking priv->end to the raw hardware return value recalculates
priv->size without adding those 4 bytes back.

This can cause the software ring buffer math to wrap before reading the
final 4 bytes of trace data written by the hardware, leading to data loss.

> +	}
> +
> +	return 0;
> +}
> +
> +static int rvtrace_ramsink_setup(struct gtrace_component *comp)
> +{
> +	struct rvtrace_ramsink_priv *priv;
> +	u32 trram_ctrl;
> +	int ret;
[ ... ]
> +	trram_ctrl = gtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	trram_ctrl |= priv->mode << RVTRACE_RAMSINK_CTRL_MODE_SHIFT;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Should the existing bits be cleared before ORing the new values?

Because the driver doesn't clear the existing bits, a previously set bit
might remain set even if the new mode evaluates to 0, leaving the hardware
improperly configured.

> +	gtrace_write32(comp->pdata, trram_ctrl, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	trram_ctrl = gtrace_read32(comp->pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	dev_dbg(&comp->dev, "mode: %s\n", (trram_ctrl >> RVTRACE_RAMSINK_CTRL_MODE_SHIFT) & 0x1 ?
> +		 "SMEM" : "SRAM");
> +
> +	trram_ctrl |= priv->stop_on_wrap << RVTRACE_RAMSINK_CTRL_STP_WRAP_SHIFT;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Similarly, does the stop-on-wrap bitfield need to be cleared before
being updated?

> +	gtrace_write32(comp->pdata, trram_ctrl, RVTRACE_COMPONENT_CTRL_OFFSET);
> +
> +	ret = rvtrace_ramsink_setup_buf(comp, priv);
> +	if (!ret)
> +		dev_set_drvdata(&comp->dev, priv);
> +
> +	return ret;
> +}
> +
> +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->size, priv->va, priv->start);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
As mentioned earlier, if priv->va, priv->start, or priv->size were modified
in rvtrace_ramsink_setup_buf(), won't this pass illegally shifted pointers
to the DMA API and cause a kernel panic?

> +}
[ ... ]
> +static const struct gtrace_component_id rvtrace_ramsink_ids[] = {
> +	{ .type = GTRACE_RVTRACE_RAMSINK,
> +	  .version = rvtrace_component_mkversion(1, 0), },
> +	{},
> +};
        ^^

[Severity: Medium]
Should this ID table be exported?

Without a MODULE_DEVICE_TABLE declaration, the driver will not be
autoloaded when compatible hardware is present on the system.

> +
> +static struct gtrace_driver rvtrace_ramsink_driver = {
> +	.id_table = rvtrace_ramsink_ids,
> +	.copyto_auxbuf = rvtrace_ramsink_copyto_auxbuf,
> +	.stop = rvtrace_ramsink_stop,
> +	.start = rvtrace_ramsink_start,
> +	.probe = rvtrace_ramsink_probe,
> +	.remove = rvtrace_ramsink_remove,
> +	.driver = {
> +		.name = "rvtrace-ramsink",
> +	},
> +};

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