Harden untrusted input handling - #696
Open
bentheredonethat wants to merge 13 commits into
Open
Conversation
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>
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>
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>
_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>
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>
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>
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>
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>
_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>
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>
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>
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>
fixup for style check - this top commit will go away once other PRs are merged Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
bentheredonethat
force-pushed
the
Harden-untrusted-input-handling
branch
from
August 26, 2026 23:37
e2e5aec to
aa885f2
Compare
Collaborator
|
@bentheredonethat |
Contributor
Author
|
@arnopo correct static analysis yielded a collection of vulnerabilities. |
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
2 participants
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.
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: