Fix #100: harden RPMsg buffer validation

[pratheesh]

Summary

• Validate RPMsg send descriptors before copying payload data into virtqueue buffers
• Validate RPMsg receive descriptor size, payload size, and caller buffer capacity before copying into the caller buffer
• Tighten virtqueue descriptor head bounds checks and publish used-ring entries before incrementing used->idx
• Update RPMsg examples to pass the receive buffer capacity through len before each receive call

Fixes #100.

Verification

• git show --check --stat ea33ad99
• Host GCC syntax check of source/rpmsg/pru_rpmsg.c and source/rpmsg/pru_virtqueue.c with a temporary pru/io.h stub; only expected 64-bit host pointer-size warnings from existing PRU address casts
• Fork Makefile CI: https://github.com/pratheesh/open-pru/actions/runs/27571640508
• Fork CCS Build: https://github.com/pratheesh/open-pru/actions/runs/27571640514
• Fork GNU Toolchain CI: https://github.com/pratheesh/open-pru/actions/runs/27571640515

Notes

• This intentionally keeps the existing pru_rpmsg_receive() uint16_t *src / uint16_t *dst API to avoid an ABI/API break. The issue's width question remains a possible follow-up if maintainers want a breaking API update.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (1)

Context used

✅ Tickets: 🎫 Review Linux RPMsg library design

1. pru_virtqueue_memory_barrier() no-op non-GNU 📎 Requirement gap ☼ Reliability

Description

pru_virtqueue_add_used_buf() now relies on pru_virtqueue_memory_barrier() to preserve used-ring
publish ordering, but that barrier is compiled out when __GNUC__ is not defined (e.g., TI clpru
builds). As a result, the host may observe used->idx updated before the corresponding
used->ring[] entry is visible, creating a synchronization race and failing the required ordering
guarantee.

Code

source/rpmsg/pru_virtqueue.c[R59-64]

+static inline void pru_virtqueue_memory_barrier(void)
+{
+#ifdef __GNUC__
+	__asm__ __volatile__ ("" : : : "memory");
+#endif
+}

Evidence

PR Compliance ID 6 requires a memory barrier before incrementing used->idx, and the implementation
in pru_virtqueue_add_used_buf() does call pru_virtqueue_memory_barrier() prior to updating
used->idx. However, the helper only emits a compiler barrier under __GNUC__ and becomes empty
otherwise; since the repository’s rpmsg library is built with the TI clpru toolchain (per the rpmsg
makefile), the intended ordering between writing used_elem->id/len into used->ring[...] and then
incrementing used->idx is not actually enforced for the primary non-GNU build.

Add memory barrier before incrementing used->idx in pru_virtqueue_add_used_buf
source/rpmsg/pru_virtqueue.c[59-64]
source/rpmsg/pru_virtqueue.c[135-140]
source/rpmsg/pru_virtqueue.c[115-140]
source/rpmsg/makefile[20-56]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pru_virtqueue_add_used_buf()` depends on `pru_virtqueue_memory_barrier()` to ensure the `used->ring[...]` entry (including `used_elem->id/len`) is visible before incrementing `used->idx`, but `pru_virtqueue_memory_barrier()` is a no-op when `__GNUC__` is not defined (e.g., TI clpru). This removes the intended publish-ordering guarantee and can allow the host to observe `used->idx` update before the corresponding ring entry is visible.

## Issue Context
This repository supports non-GNU toolchains (the code already has `#ifdef __GNUC__` vs `#else` paths), and the rpmsg library is built with `clpru` via `source/rpmsg/makefile`. Because `used` and `used_elem` are not `volatile`, a missing barrier on non-GNU compilers provides no compile-time ordering guarantee and can permit reordering/caching of stores, violating the compliance requirement for a barrier before `used->idx` is incremented.

## Fix Focus Areas
- source/rpmsg/pru_virtqueue.c[59-64]
- source/rpmsg/pru_virtqueue.c[115-140]
- source/rpmsg/pru_virtqueue.c[135-140]
- source/rpmsg/makefile[20-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Wrong used length 🐞 Bug ≡ Correctness

Description

pru_rpmsg_send() returns the TX descriptor to the used ring with len=msg_len (descriptor size)
rather than the bytes actually written (RPMsg header + payload). This violates vring_used_elem.len
semantics and can cause the host to treat unwritten/stale bytes as part of the message.

Code

↗ source/rpmsg/pru_rpmsg.c

	head = pru_virtqueue_get_avail_buf(virtqueue, (void **)&msg, &msg_len);

Evidence

The virtio ring ABI in this repo explicitly defines used-element length as bytes written, but
pru_rpmsg_send() passes the descriptor's full length (msg_len) to pru_virtqueue_add_used_buf().

source/include/linux/pru_virtio_ring.h[79-85]
source/rpmsg/pru_rpmsg.c[82-132]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pru_rpmsg_send()` reports the *descriptor capacity* (`msg_len`) as the used length, but virtio defines `vring_used_elem.len` as the number of bytes actually written. This can mis-size the message on the host side.

### Issue Context
- `vring_used_elem.len` is defined as: "Total length of the descriptor chain which was used (written to)".
- In `pru_rpmsg_send()`, only `sizeof(struct pru_rpmsg_hdr) + len` bytes are actually written/valid.

### Fix Focus Areas
- source/rpmsg/pru_rpmsg.c[111-132]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Invalid head stalls queue 🐞 Bug ☼ Reliability

Description

pru_virtqueue_get_avail_buf() returns PRU_VIRTQUEUE_INVALID_HEAD before incrementing last_avail_idx,
so the same bad avail-ring entry will be reread on every call. A single malformed head can
permanently prevent the queue from making progress even if later entries are valid.

Code

source/rpmsg/pru_virtqueue.c[R101-107]

+	head = avail->ring[vq->last_avail_idx & (vq->vring.num - 1)];
+
+	if ((uint32_t)head >= vq->vring.num)
+		return PRU_VIRTQUEUE_INVALID_HEAD;
+
+	vq->last_avail_idx++;

Evidence

The code reads avail->ring[last_avail_idx & mask] and returns invalid-head without incrementing
last_avail_idx, ensuring the same ring slot is retried indefinitely; the header comment also
describes last_avail_idx as the consumed-buffer cursor.

source/rpmsg/pru_virtqueue.c[81-113]
source/include/linux/pru_virtqueue.h[83-99]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pru_virtqueue_get_avail_buf()` now validates `head` before advancing `vq->last_avail_idx`. If `head` is invalid, the cursor never advances, so the queue can livelock on the same entry.

### Issue Context
This is a behavioral change from the previous post-increment form (`vq->last_avail_idx++`) and it means one bad entry blocks all subsequent ones.

### Fix Focus Areas
- source/rpmsg/pru_virtqueue.c[91-113]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

[qodo-code-review]

PR Summary by Qodo

Harden RPMsg buffer and virtqueue descriptor validation
🐞 Bug fix 📝 Documentation 🕐 20-40 Minutes

Walkthroughs

Description

• Validate RPMsg send/receive descriptor and payload sizes before memcpy operations.
• Tighten virtqueue head bounds checks and publish used-ring entries before idx increments.
• Update RPMsg echo examples to pass receive buffer capacity via len each iteration.

Diagram

graph TD
  A["RPMsg API"] --> B{"Validate sizes"} --> C["Virtqueue get_avail"] --> D["Copy payload"] --> E["Virtqueue add_used"] --> F["Kick host"]
  subgraph "Callers"
    G["RPMsg examples"]
  end
  G --> A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Break API: separate in-capacity and out-length params

• ➕ Eliminates len in/out ambiguity (clearer contract)
• ➕ Allows wider types (size_t/uint32_t) for capacity/length
• ➕ Reduces risk of callers forgetting to reset len before receive
• ➖ ABI/API breaking change for existing firmware
• ➖ Requires updating downstream examples and any external users

2. Use platform-wide memory barrier abstraction

• ➕ Can provide correct ordering on non-GNU toolchains/CCS as well
• ➕ Makes ordering intent explicit and testable
• ➖ Needs platform/compiler-specific implementations
• ➖ Risk of over/under-fencing without hardware-specific guidance

Recommendation: Current approach is a good non-breaking fix for #100: it adds concrete size/capacity checks and closes a subtle used-ring publication ordering hazard with minimal surface-area change. Consider a follow-up that modernizes the receive API to separate buffer capacity from returned payload length, and (if needed for non-GNU builds) centralizes the virtqueue memory barrier behind a portable abstraction.

File Changes

Bug fix (2)

pru_rpmsg.c Add RPMsg send/receive descriptor and payload bounds checks +18/-3

Add RPMsg send/receive descriptor and payload bounds checks

• pru_rpmsg_send() now distinguishes invalid virtqueue heads and validates descriptor capacity for header+payload before copying, returning the buffer as used on failure. pru_rpmsg_receive() validates descriptor length, header fit, payload fit within descriptor, and payload fit within caller-provided capacity before copying; on validation failure it releases the buffer and returns PRU_RPMSG_BUF_TOO_SMALL.

source/rpmsg/pru_rpmsg.c

---

pru_virtqueue.c Harden virtqueue head bounds and used-ring publication ordering +17/-3

Harden virtqueue head bounds and used-ring publication ordering

• Validates avail ring heads before using them and only then increments last_avail_idx. Tightens add_used head checks (including negative heads) and writes used->ring entry before incrementing used->idx, adding a compiler memory barrier for GNU builds to prevent reordering.

source/rpmsg/pru_virtqueue.c

---

Documentation (3)

main.c Reset receive buffer capacity before each pru_rpmsg_receive() +2/-0

Reset receive buffer capacity before each pru_rpmsg_receive()

• Sets len = sizeof(payload) before entering the receive loop and after each echoed message. This matches the updated pru_rpmsg_receive() contract where *len is an input capacity and output payload length.

examples/gcc_rpmsg_echo_linux/main.c

---

main.c Pass buffer capacity via len for repeated receives +2/-0

Pass buffer capacity via len for repeated receives

• Initializes len to the payload buffer size before the receive loop and resets it after each send. Prevents subsequent receives from using the prior message length as the new capacity.

examples/rpmsg_echo_linux/firmware/main.c

---

pru_rpmsg.h Clarify pru_rpmsg_receive() len parameter and error semantics +7/-3

Clarify pru_rpmsg_receive() len parameter and error semantics

• Updates the API comment to state that *len is caller-provided buffer capacity on entry and payload length on success. Documents PRU_RPMSG_BUF_TOO_SMALL for undersized caller buffers and undersized virtqueue descriptors.

source/include/linux/pru_rpmsg.h

---

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

[qodo-code-review]

1. pru_virtqueue_memory_barrier() no-op non-gnu 📎 Requirement gap ☼ Reliability

pru_virtqueue_add_used_buf() now relies on pru_virtqueue_memory_barrier() to preserve used-ring
publish ordering, but that barrier is compiled out when __GNUC__ is not defined (e.g., TI clpru
builds). As a result, the host may observe used->idx updated before the corresponding
used->ring[] entry is visible, creating a synchronization race and failing the required ordering
guarantee.

Agent Prompt

## Issue description
`pru_virtqueue_add_used_buf()` depends on `pru_virtqueue_memory_barrier()` to ensure the `used->ring[...]` entry (including `used_elem->id/len`) is visible before incrementing `used->idx`, but `pru_virtqueue_memory_barrier()` is a no-op when `__GNUC__` is not defined (e.g., TI clpru). This removes the intended publish-ordering guarantee and can allow the host to observe `used->idx` update before the corresponding ring entry is visible.

## Issue Context
This repository supports non-GNU toolchains (the code already has `#ifdef __GNUC__` vs `#else` paths), and the rpmsg library is built with `clpru` via `source/rpmsg/makefile`. Because `used` and `used_elem` are not `volatile`, a missing barrier on non-GNU compilers provides no compile-time ordering guarantee and can permit reordering/caching of stores, violating the compliance requirement for a barrier before `used->idx` is incremented.

## Fix Focus Areas
- source/rpmsg/pru_virtqueue.c[59-64]
- source/rpmsg/pru_virtqueue.c[115-140]
- source/rpmsg/pru_virtqueue.c[135-140]
- source/rpmsg/makefile[20-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[pratheesh]

Addressed the memory-barrier ordering gap (736218c).

pru_virtqueue_memory_barrier() was a no-op for non-GNU toolchains, so the used-ring publish ordering before used->idx++ in pru_virtqueue_add_used_buf() wasn't enforced when built with clpru. Added an __asm(" ") for the non-GNU path — the TI compiler treats an empty asm statement as a volatile optimization barrier it will not reorder memory accesses across, which is sufficient on the in-order PRU core (no hardware store reordering). Verified it compiles with clpru 2.3.3; Makefile CI green.