Page MenuHomeFreeBSD

bhyve e1000: Skip packets with a small header.
ClosedPublic

Authored by jhb on Aug 12 2022, 7:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 9, 4:38 PM
Unknown Object (File)
Mon, Jul 1, 7:57 AM
Unknown Object (File)
Fri, Jun 28, 7:32 AM
Unknown Object (File)
Tue, Jun 25, 5:42 AM
Unknown Object (File)
Sat, Jun 22, 10:20 AM
Unknown Object (File)
Jun 18 2024, 3:29 AM
Unknown Object (File)
May 13 2024, 7:26 AM
Unknown Object (File)
May 12 2024, 8:44 PM

Details

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Aug 12 2022, 7:07 PM
usr.sbin/bhyve/pci_e82545.c
236

this is because we had no WPRINTFs w/o params before?

usr.sbin/bhyve/pci_e82545.c
1161

should iovcnt >= I82545_MAX_TXSEGS also set invalid?

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.

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.

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.

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.

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)

This revision is now accepted and ready to land.Aug 15 2022, 8:26 PM
This revision was automatically updated to reflect the committed changes.