Repository navigation
Harden untrusted input rpmsg virtio - #702
Merged
arnopo merged 9 commits intoOct 19, 2026
Merged
Conversation
arnopo
reviewed
Sep 15, 2026
arnopo
left a comment
arnopo
left a comment
Collaborator
There was a problem hiding this comment.
one extra comment else LGTM
bentheredonethat
force-pushed
the
Harden-untrusted-input-rpmsg-virtio
branch
from
September 15, 2026 14:02
95f52bd to
be4d485
Compare
arnopo
approved these changes
Sep 21, 2026
arnopo
left a comment
arnopo
left a comment
Collaborator
There was a problem hiding this comment.
It might be too much AI verbosity in inline comment, but that’s just my feeling. If other reviewers don’t report it, we can keep it as is.
The remote peer controls every byte of a name service message name. If the name fills the wire field without a NUL terminator, endpoint lookup and application callbacks read past the copied name when they treat it as a C string. Terminate the copied name inside the name field before it is used, as upstream Linux RPMsg does. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5
When the source is smaller than the destination and contains a NUL, safe_strcpy() includes the copied terminator in its zero-fill length. The resulting memset() writes one byte beyond the destination buffer. Calculate the fill length from the advanced destination pointer. This ensures only bytes remaining inside the destination are cleared. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5
virtqueue_create() takes the vring alignment from its caller without checking it. vring_init() rounds the used ring address with a ~(align - 1) mask, so a zero alignment places the used ring at NULL. Reject a zero alignment in the virtqueue constructor and document the requirement. Check the ring pointer unconditionally as well, since VQ_PARAM_CHK() compiles out unless VQ_DEBUG is enabled. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5
The remote peer controls both the used-ring length and the payload length in the rpmsg header. Trusting either value can make cache invalidation and endpoint callbacks access beyond the receive buffer. Cap the driver-side length to the locally allocated buffer size before cache invalidation. Deliver a buffer to an endpoint only when the header and the announced payload fit in the reported length, and release the buffer through the existing path otherwise. Move the endpoint pointer from function scope into the receive loop body and initialize it at its declaration. The endpoint lookup is now conditional, so a function-scope variable would retain the endpoint resolved by a previous iteration whenever the length checks reject a buffer, and that already unreferenced pointer would be dispatched to and unreferenced a second time. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5
Name service messages are allocated on the stack and copied in full to the remote peer. safe_strcpy() does not clear bytes after a short name when source and destination sizes match. This can disclose stack contents. Zero-initialize the message before populating its fields. Unused name bytes then never cross the shared-memory trust boundary. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5
virtqueue_create() rejects a zero vring alignment but accepts any other value. vring_init() rounds the used ring address with a ~(align - 1) mask, which only clears low order bits when the alignment is a power of two. A value such as 0x80000001 makes the mask clear high order bits of the computed address instead, placing the used ring outside the vring memory while the local core keeps dereferencing it. Require the alignment to be a power of two in the virtqueue constructor and document the requirement. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5
virtqueue_get_buffer() takes the descriptor index from the used ring, which the remote processor writes, and uses it to index vq_descx[] without any bounds check. A remote that reports an index above vq_nentries makes the driver read a cookie from past the end of the array and then write NULL over that same location. vq_ring_free_chain() is reached first and only guards the index with VQ_RING_ASSERT_VALID_IDX(), which expands to nothing unless VQ_DEBUG is enabled, so it does not prevent the access in normal builds. Drop buffers whose descriptor index is out of range, and bound the descriptor chain walk so it stays inside the descriptor table when the assert is compiled out. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5
In the device role, the peer supplies the descriptor address and length. An address near the end of shared memory with an oversized length can invalidate memory outside that region. A buffer shorter than 12 bytes also lets the reserved field assignment write beyond the buffer before the existing complete-header and payload checks run. Require device receive buffers to fit entirely within shared memory and contain a complete header before invalidating or accessing their data. Return malformed descriptors with zero consumed length without touching their buffers. Recycle short driver buffers before writing the header, and reject available descriptor indices outside the descriptor table. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-6
bentheredonethat
force-pushed
the
Harden-untrusted-input-rpmsg-virtio
branch
from
October 5, 2026 14:43
e90b35f to
0ea29fd
Compare
The receive buffer validation change left two consecutive blank lines before cache invalidation. The strict checkpatch run used by CI reports LINE_SPACING for this section. Remove the extra blank line to satisfy the compliance style check. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-6
tnmysh
approved these changes
Oct 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.