https://bugs.dpdk.org/show_bug.cgi?id=2013
Bug ID: 2013
Summary: memif: accepts rings outside the region
Product: DPDK
Version: 26.11
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: normal
Priority: Normal
Component: ethdev
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---
Group: security
memif_msg_receive_add_ring() in drivers/net/memif/memif_socket.c checks
only that the ring index is below the configured ring count. Four
things a client controls are taken unchecked.
1. Ring index may repeat, and the counter is incremented unconditionally
The handler checks ar->index >= pmd->cfg.num_c2s_rings (or the s2c
equivalent) and then increments pmd->run.num_c2s_rings. Nothing requires
rings to be added in order, or at all, or only once. A client that sends
ADD_RING for index 0 repeatedly drives pmd->run.num_c2s_rings past
pmd->cfg.num_c2s_rings, and code that later iterates over the run
counters then indexes past the allocated queue arrays. This is the item
in this bug with a direct memory-safety consequence and it is worth
treating as the primary one.
2. Ring size is not bounded
ar->log2_ring_size is not checked against the maximum the server
advertised in its hello, so the ring described can be far larger than
anything the server expects.
3. The referenced region is not checked to exist, and the ring is not
checked to fit inside it
ar->region is not checked against the regions actually added, and
mq->offset = ar->offset is taken as-is. Nothing verifies that the ring
header plus its descriptor table fit inside the named region, or that
the offset is naturally aligned. A bad region index or offset makes
mq->ring in memif_connect() point outside any mapping; the cookie read
there is then an out-of-bounds access, and the ring head and tail are
subsequently dereferenced through the same pointer.
4. private_hdr_size is not rejected
Private headers are not supported, but a non-zero ar->private_hdr_size
is accepted, which puts the descriptor table somewhere other than where
the driver computes it to be.
Suggested fix
-------------
- require rings to be added in order, exactly once: reject unless
ar->index equals the current run counter, and increment only after
all checks pass;
- bound ar->log2_ring_size by the value actually advertised in the
hello, rather than by a separate constant that can drift from it;
- reject non-zero ar->private_hdr_size;
- require the named region to exist (regions are added before rings)
and require sizeof(memif_ring_t) + sizeof(memif_desc_t) << log2_size
to fit within region_size at the given offset, with the offset
naturally aligned so the atomic head and tail accesses do not fault
on platforms that require aligned access.
On alignment: both DPDK and VPP place rings on a cache line boundary in
practice, but the proposed fix only enforces 8-byte alignment, to avoid
rejecting configurations that are legal under the wire protocol as it
stands. Whether a stricter alignment can be relied on is one of the
interop questions that should be settled with VPP and libmemif.
Note for reviewers: ar->offset is uint32_t (memif_region_offset_t), so
offset + ring_size evaluated in 64-bit arithmetic cannot wrap today.
That is worth a comment in the code, so that a future widening of the
wire type does not silently turn the bound check into a bypass.
A candidate patch exists privately and has not been posted. It can be
shared with the security team on request.
Reported by Arthur Chan <[email protected]> (Ada Logics), via
fuzzing.
--
You are receiving this mail because:
You are the assignee for the bug.
