A fuzzing effort by Arthur Chan turned up a number of ways for a memif
client to upset a memif server. Taken together they are not isolated
bugs so much as one systematic gap, which this series addresses.
memif is asymmetric. On connect the server hands its region file
descriptors to the peer, so a client can reasonably trust the server it
connected to, but the server cannot trust the client. The shared rings
and their descriptors stay writable by the peer for the life of the
connection, and control channel messages arrive before anything has been
validated. The driver was written as if both ends were cooperative: it
turned client-supplied region sizes, ring geometry and buffer
descriptors into pointers and lengths without checking them. That is
what the fuzzer found, from several directions.
Patches 3 to 5 do the validation (Bugzilla 2010, 2011, 2012, 2013, 2018,
2019) and are marked for stable, as is the statistics fix in patch 2.
Patch 6 adds a two-process testpmd connectivity test, which is also the
harness an adversarial peer can be driven from. Patches 1, 2 and 7 are
unrelated to the fuzzing and bundled because they touch the same driver.
The PMD has effectively been unmaintained for some time, which is why
these have sat. A call for a new maintainer went out and Sriram
Yagnaraman has volunteered; he has reviewed and tested this series.
Interoperability
----------------
These checks were written against the VPP plugin and libmemif so that a
hardened DPDK server does not disconnect conforming clients. Two are
deliberately looser than they could be:
- A missing F_SEAL_SHRINK on a region is a warning, not a rejection.
libmemif seals region 0, but hugepage-backed regions cannot be
sealed, and a memfd created without MFD_ALLOW_SEALING is
indistinguishable from one whose owner chose not to seal. The fstat
size check covers the same ground without breaking those clients.
- Ring offsets are only required to be 8-byte aligned, though
implementations use 64 in practice, to avoid rejecting small-ring
setups.
If anyone knows of a stricter guarantee that can be relied on here, I
would rather tighten these.
Not fixed here
--------------
Some reports need agreement on the wire protocol rather than a local fix:
- Bugzilla 2014: hello advertises the driver-wide maximum ring count
rather than the device's configured count. The obvious one-line fix
does not work: hello is built from the control channel alone, and the
device is not associated with it until init, which arrives after
hello has been sent. This means changing when ring counts are
negotiated.
- Bugzilla 2015: ring semantics differ from the VPP implementation.
- Bugzilla 2017: the trust model and the normative requirements for a
conforming client are not written down anywhere. That amounts to
specifying the protocol, better done with a maintainer to agree it.
Three more pre-existing ways for a client to upset a server turned up
while writing this. All are structural rather than local, so they are
listed here rather than tacked onto the end of the series:
- memif_disconnect() unmaps the regions while the data path may still
be inside a burst function. The burst functions test the connected
flag only on entry, so a client that disconnects under load can
already fault the server. Fixing it means waiting for lcores to leave
the burst functions, or deferring the unmap to a quiescent point.
- Control messages are dispatched on type alone, with no check of role
or connection state. A client that sends init then hello makes the
server run the client-only setup path. This wants an accept table by
role and state in memif_msg_receive().
- Queues the client does not add keep the ring geometry from the
previous connection: disconnect does not reset it and connect only
walks the negotiated ring counts. An offset validated against the old
region can then be used against a new, smaller one. Ties into 2014.
One limitation within the series: a secondary process that sees a bad
descriptor counts the error but cannot tear the connection down, since
only the primary owns the control channel. That needs an mp message.
Reported-by: Arthur Chan <[email protected]>
Stephen Hemminger (7):
maintainers: update for memif driver
net/memif: fix issues in statistics
net/memif: validate peer descriptors
net/memif: validate control channel requests
net/memif: validate descriptor length in zero-copy mode
net/memif: add server/client connectivity test
doc: clarify memif secret is not access control
.ci/linux-build.sh | 1 +
.mailmap | 1 +
MAINTAINERS | 2 +
devtools/test-memif.sh | 169 +++++++++++++
doc/guides/nics/memif.rst | 20 +-
drivers/net/memif/memif_socket.c | 129 ++++++++--
drivers/net/memif/rte_eth_memif.c | 397 +++++++++++++++++++++++++-----
drivers/net/memif/rte_eth_memif.h | 1 +
8 files changed, 635 insertions(+), 85 deletions(-)
create mode 100755 devtools/test-memif.sh
--
2.53.0
