Harden untrusted input handling by bentheredonethat · Pull Request #696 · OpenAMP/open-amp

bentheredonethat · 2026-08-26T23:32:02Z

This series hardens OpenAMP paths that consume firmware-controlled
data, messages from a remote processor, and caller-provided lengths.
These inputs cross trust boundaries in AMP systems. They must be
validated before memory access, allocation, or callback dispatch.

The remoteproc changes validate ELF section-name offsets and header
table entry sizes. They bound each resource-table entry before its
handler runs and reject unsafe vring alignments before size or pointer
calculations. The checks cover fixed resources and variable-length
VDEV vring and configuration data.

The RPMsg RPC and proxy changes reject malformed message lengths.
They initialize omitted request bytes, report callback payload lengths
without protocol headers, and bound retarget read and write copies to
both source and destination capacities. Name-service strings are
terminated before being passed to application callbacks.

The remaining changes correct the safe_strcpy() zero-fill off-by-one.
They also make virtio MMIO queue setup honor its allocated capacity
instead of appending beyond it.

Validation performed:

git diff --check on every patch
CMake Debug build against local Libmetal with -Wall -Wextra
static and shared OpenAMP library targets built successfully
CTest invoked; this configuration provides no tests

arnopo · 2026-08-28T09:57:57Z

@bentheredonethat
Is these updates result from a report from a tools?

bentheredonethat · 2026-08-28T13:57:01Z

@arnopo correct static analysis yielded a collection of vulnerabilities.

arnopo · 2026-09-01T08:00:51Z

@arnopo correct static analysis yielded a collection of vulnerabilities.

In such case could you used the assisted by new tag described here: https://github.com/OpenAMP/open-amp/blob/main/README.md#assisted-by

arnopo · 2026-09-01T08:09:17Z

-			    &name, sizeof(name));
+			    &name, sizeof(ns_msg->name));
+	/* Ensure the remote-supplied service name is NUL-terminated. */
+	name[RPMSG_NAME_SIZE] = '\0';


No sure that this prevent application issue.
the RPMSG_NAME_SIZE define the max size.
If application define a char name[RPMSG_NAME_SIZE] table this not prevent an overflow in application

from my POV, the fix should be in application not here

This change is intended to protect the internal rpmsg_virtio callback path from treating an unterminated remote-supplied field as a C string. The wire-format field can contain exactly RPMSG_NAME_SIZE non-NUL bytes, so the local buffer needs one additional byte for a terminator before it is passed to endpoint lookup or application callbacks.

I agree that this does not by itself prevent an application from copying the name into char name[RPMSG_NAME_SIZE] and then using it as a NUL-terminated string. Applications must still allocate RPMSG_NAME_SIZE + 1 bytes or use bounded string operations. This change addresses the separate library-side out-of-bounds-read risk and ensures the name delivered by this callback is terminated.

arnopo · 2026-09-01T08:19:36Z

+	if (size < d_size) {
+		/* Clear only bytes remaining after the copied data. */
+		memset(dst, '\0', d_size - (size_t)(dst - d));
+	} else {


nitpicking: remove unecessary braces to better see you real update

arnopo · 2026-09-01T08:26:15Z

+				if (tmplen > payload_capacity)
+					tmplen = payload_capacity;
+				if (tmplen > (size_t)buflen)
+					tmplen = (size_t)buflen;


what's happen if payload_capacity < buflen < tmplen ?
In such cas we should have tmplen = payload_capacity, right?

arnopo · 2026-09-01T08:30:22Z

-	vdev->vrings_info = metal_allocate_memory(sizeof(struct virtio_vring_info) * vq_num);
-	/* TODO: handle error case */
+	if (!vdev || !vqs || vq_num <= 0)
+		return;


I wonder if here we should return an error. This would modify the function prototype but it should remains compatible with legacy

arnopo · 2026-09-01T08:33:34Z

-	vdev->vrings_info[vdev->vrings_num].vq = vq;
-	vdev->vrings_num++;
+	/* Store the configured queue in its registered vring slot. */
+	vdev->vrings_info[idx].vq = vq;


why vdev->vrings_num++; line has been removed?

arnopo · 2026-09-01T08:35:35Z


-	if (!vdev || !names || !vdev->vrings_info)
+	if (!vdev || !names || !vdev->vrings_info ||
+	    nvqs > vdev->vrings_num)


squash on one line

arnopo · 2026-09-01T08:37:16Z

 #define	RPROC_MAX_VRING_DESC	USHRT_MAX

+/* Maximum supported alignment for a remoteproc vring. */
+#define RPROC_MAX_VRING_ALIGN	4096U


What is the rational behin this limitation?
if not mandatory I prefer to avoid it.

arnopo · 2026-09-01T08:43:49Z

 	void *vaddr;

-	/** Vring alignment. */
+	/** Vring alignment; must be a nonzero power of two. */


are you sure that it can be null ? It should work...

arnopo · 2026-09-01T08:50:11Z


-	VQ_PARAM_CHK(ring == NULL, status, ERROR_VQUEUE_INVLD_PARAM);
+	if (!ring)
+		return ERROR_VQUEUE_INVLD_PARAM;


why not keeping the initial VQ_PARAM_CHK(ring == NULL, status, ERROR_VQUEUE_INVLD_PARAM); ?

The RPC callback does not receive the request length, so it may inspect bytes beyond a short message. Those bytes currently come from an uninitialized stack buffer. A message shorter than the function ID can also make the dispatch path read uninitialized data. Reject messages that do not contain a complete function ID and zero-initialize the request buffer so callbacks never consume stale stack contents from bytes omitted by the remote peer. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

Section names are resolved by adding the firmware-provided sh_name offset to the loaded section string table and passing the result to strcmp(). A malformed offset or unterminated entry can therefore cause an out-of-bounds read. Track the loaded string table size and require each candidate name, including its terminator, to fit within the remaining table bytes before comparing it. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

The remote peer controls every byte of a name service message name. If the name fills the wire field without a NUL terminator, application callbacks can read beyond the local stack buffer when treating it as a C string. Reserve an extra byte in the local buffer and explicitly terminate the copied name before endpoint lookup or application callbacks use it. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

_write() copies a caller-controlled length into a fixed-size stack buffer without checking that the RPC header and payload fit. Negative lengths also become large unsigned memcpy() sizes, and the stdout NUL terminator is written one byte beyond its intended position. Reject invalid lengths before constructing the request. Include the optional terminator in the capacity check and place the terminator immediately after the copied payload. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

The loader allocates program and section header tables using the entry sizes supplied by the firmware. Later users index those allocations as arrays of native ELF header structures. Smaller entry sizes therefore under-allocate the tables and make indexed access read beyond them. Require each nonempty table to use the expected ELF32 or ELF64 entry size before allocating or copying loader state. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

The RPC client reads the reply ID and status without checking that the remote message contains the fixed reply header. It also passes the total message length to callbacks that receive a parameters pointer, making the reported length include the header bytes. Reject replies shorter than the fixed header and pass callbacks only the number of bytes that follow it. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

The RPC client copies caller-provided parameters into a fixed-size stack buffer without checking whether the complete request fits. Oversized requests can therefore overwrite the caller's stack frame. Reject requests that exceed the remaining parameter capacity before copying them, validate nonempty parameter pointers, and document the public API limit. 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

_read() converts the peer-provided unsigned data length to a signed integer and bounds it only against the caller's buffer size. Large values can bypass that comparison or make memcpy() read beyond the fixed response buffer. Reject nonpositive destination sizes, retain the peer length as an unsigned value, and clamp it to both the response payload capacity and the caller's buffer before copying. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

MMIO registration allocates vrings_num entries and records the value as the device's vring count. Queue setup then uses the count as an append index, so its first store is immediately beyond the allocation. Treat vrings_num as the registered capacity, validate setup and bulk creation against it, and store each configured queue in its indexed slot. Also validate registration inputs and allocation success. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

The remote resource table controls vring alignment, which is used in pointer rounding without validation. Zero or non-power-of-two values can redirect the used-ring pointer outside the vring allocation. Reject unsupported alignments before calculating remoteproc vring sizes and before storing their metadata. Enforce the alignment requirement again in the generic virtqueue constructor. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

Resource offsets from a copied firmware table are dereferenced without bounds checks when no I/O region is present. Handlers can then access memory beyond the table through a VDEV's flexible vring array. Validate offset-array arithmetic and require every entry to remain inside the table before dispatch. Check fixed sizes, VDEV vrings and config data, and vendor lengths using overflow-safe subtraction. 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. Drop frames with truncated headers or payload lengths beyond the reported buffer and return them to the virtqueue. 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

fixup for style check - this top commit will go away once other PRs are merged Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

bentheredonethat force-pushed the Harden-untrusted-input-handling branch from e2e5aec to aa885f2 Compare August 26, 2026 23:37

arnopo requested review from arnopo, edmooring and tnmysh August 28, 2026 09:50

arnopo reviewed Sep 1, 2026

View reviewed changes

bentheredonethat added 15 commits September 1, 2026 06:16

fixup! remoteproc: Validate ELF header table entry sizes

9b24020

fixup for style check - this top commit will go away once other PRs are merged Signed-off-by: Ben Levinsky <ben.levinsky@amd.com> Assisted-by: Codex:GPT-5

bentheredonethat force-pushed the Harden-untrusted-input-handling branch from aa885f2 to 9b24020 Compare September 1, 2026 14:25

Uh oh!

Conversation

bentheredonethat commented Aug 26, 2026

Uh oh!

arnopo commented Aug 28, 2026

Uh oh!

bentheredonethat commented Aug 28, 2026

Uh oh!

arnopo commented Sep 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnopo Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnopo Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arnopo Sep 1, 2026 •

edited

Loading

arnopo Sep 1, 2026 •

edited

Loading