bhyve e1000: Skip packets with a small header.
ClosedPublic
Actions

Authored by jhb on Aug 12 2022, 7:07 PM.

Details

Reviewers

grehan
markj
bz

Group Reviewers

bhyve

Commits

rG2d349fd22c97: bhyve e1000: Skip packets with a small header.
rGff5d46d7f9a1: bhyve e1000: Skip packets with a small header.
rGfa46f3704b76: bhyve e1000: Skip packets with a small header.

Summary

Certain operations such as checksum insertion and VLAN insertion
require the device model to rewrite the packet header. The first step
in rewriting the packet header is to copy the existing packet header
from the source packet. This copy is done by copying data from an
iovec array that corresponds to the S/G entries described by transmit
descriptors. However, if the total packet length is smaller than the
headers that need to be copied as the initial template, this copy can
overflow the iovec array and use garbage values as the source pointer
to memcpy. The PR used a single descriptor with a length of 0 in its
PoC.

To fix, track the total packet length and drop requests to transmit
packets whose payload is smaller than the required header length.

While here, fix another issue where the final descriptor could have an
invalid length (too short) that could underflow 'len' when stripping
the checksum. Skip those requests instead, too.

PR: 264372
Reported by: Robert Morris <rtm@lcs.mit.edu>
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 46909
Build 43798: arc lint + arc unit

Event Timeline

jhb created this revision.Aug 12 2022, 7:07 PM

Herald added a reviewer: bhyve. · View Herald TranscriptAug 12 2022, 7:07 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

jhb requested review of this revision.Aug 12 2022, 7:07 PM

Harbormaster completed remote builds in B46909: Diff 109272.Aug 12 2022, 7:08 PM

emaste added inline comments.Aug 12 2022, 7:33 PM

usr.sbin/bhyve/pci_e82545.c
236	this is because we had no `WPRINTF`s w/o params before?

emaste added inline comments.Aug 12 2022, 7:36 PM

usr.sbin/bhyve/pci_e82545.c
1161	should iovcnt >= I82545_MAX_TXSEGS also set `invalid`?

jhb added inline comments.Aug 12 2022, 8:54 PM

usr.sbin/bhyve/pci_e82545.c
236	Yes.
1161	Right now `invalid` assumes you've already output an error message, but the existing error message for the iovcnt case is after the loop. Part of the reason this is a bit more complex is we have to finish the loop to get to the EOP descriptor instead of just jumping directly to 'done' on an error. I could perhaps move the check for iovcnt overflow and error message up here and then set invalid, but that's a bit larger of a change.

emaste added inline comments.Aug 12 2022, 8:59 PM

usr.sbin/bhyve/pci_e82545.c
1161	Ok, i see

Looks reasonable to me, but happy to have one of the requested reviewers check it over.

markj added inline comments.Aug 15 2022, 4:08 PM

usr.sbin/bhyve/pci_e82545.c
1316	Is this a functional change? I can't see how it's possible to have vlen != 0 and hdrlen == 0. Not to say that the change is incorrect, though.

emaste added inline comments.Aug 15 2022, 4:18 PM

usr.sbin/bhyve/pci_e82545.c
1316	I think that case is not possible -- I believe `vlen != 0` implies `hdrlen` is at least `ETHER_ADDR_LEN*2`. AFAICT this change is correct, but not strictly necessary.

jhb added inline comments.Aug 15 2022, 7:05 PM

usr.sbin/bhyve/pci_e82545.c
1316	I think I mostly made it for readability since the builtin_alloca() takes the combined size, and the new check I've added above takes the combined size. Possibly the VLAN insertion should also be moved inside this if as well, but that would add a lot of noise (maybe it can be done as a followup)