Bug 264567 - bhyve's e82545_transmit() can index beyond the end of the tx descriptors
Summary: bhyve's e82545_transmit() can index beyond the end of the tx descriptors
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bhyve (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: John Baldwin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-09 09:32 UTC by Robert Morris
Modified: 2023-01-20 18:35 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2022-06-09 09:32:02 UTC
When it is first executed, e82545_tx_run() passes whatever it finds in
sc->esc_TDH to e82545_transmit() as the head index, and the latter
uses the index without any check:

                dsc = &sc->esc_txdesc[head];

The guest can specify any 16-bit TDH by writing the E1000_TDH(0)
register. So it can cause e82545_transmit() to try to read a host
address up to a megabyte beyond the end of guest memory. And can cause
e82545_transmit_done() to try to write there.
Comment 1 John Baldwin freebsd_committer freebsd_triage 2022-08-19 23:07:50 UTC
Potential fix at https://reviews.freebsd.org/D36269.
Comment 2 Robert Morris 2022-08-20 09:44:16 UTC
(In reply to John Baldwin from comment #1)
D36269 does fix the problem for me.
Comment 3 commit-hook freebsd_committer freebsd_triage 2022-08-29 22:38:28 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=7afe342dcb38b624488009bb6bdfa5337e628ffc

commit 7afe342dcb38b624488009bb6bdfa5337e628ffc
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2022-08-29 22:35:15 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2022-08-29 22:36:57 +0000

    bhyve e1000: Sanitize transmit ring indices.

    When preparing to transmit pending packets, ensure that the head (TDH)
    and tail (TDT) indices are in bounds.  Note that validating values
    when they are written is not sufficient along as the transmit length
    (TDLEN) could be changed turning a value that was valid when written
    into an out of bounds value.

    While here, add further restrictions to the head register (TDH).  The
    manual states that writing to this value while transmit is enabled can
    cause unexpected behavior and that it should only be written after a
    reset.  As such, ignore attempts to write while transmit is active,
    and also ignore writes of non-zero values.  Later e1000 chipsets have
    this register as read-only.

    Also ignore any attempts to transmit packets if the transmit ring's
    size is zero.

    PR:             264567
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Reviewed by:    emaste
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D36269

 usr.sbin/bhyve/pci_e82545.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2022-11-11 01:25:31 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=0d347cf94942914c7eb15360c995df9c9091720c

commit 0d347cf94942914c7eb15360c995df9c9091720c
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2022-08-29 22:35:15 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2022-11-11 00:41:55 +0000

    bhyve e1000: Sanitize transmit ring indices.

    When preparing to transmit pending packets, ensure that the head (TDH)
    and tail (TDT) indices are in bounds.  Note that validating values
    when they are written is not sufficient along as the transmit length
    (TDLEN) could be changed turning a value that was valid when written
    into an out of bounds value.

    While here, add further restrictions to the head register (TDH).  The
    manual states that writing to this value while transmit is enabled can
    cause unexpected behavior and that it should only be written after a
    reset.  As such, ignore attempts to write while transmit is active,
    and also ignore writes of non-zero values.  Later e1000 chipsets have
    this register as read-only.

    Also ignore any attempts to transmit packets if the transmit ring's
    size is zero.

    PR:             264567
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Reviewed by:    emaste
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D36269

    (cherry picked from commit 7afe342dcb38b624488009bb6bdfa5337e628ffc)

 usr.sbin/bhyve/pci_e82545.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-11-11 01:25:33 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=93eafd4479da09eabbd447490a8661fb7616ba29

commit 93eafd4479da09eabbd447490a8661fb7616ba29
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2022-08-29 22:35:15 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2022-11-11 01:10:45 +0000

    bhyve e1000: Sanitize transmit ring indices.

    When preparing to transmit pending packets, ensure that the head (TDH)
    and tail (TDT) indices are in bounds.  Note that validating values
    when they are written is not sufficient along as the transmit length
    (TDLEN) could be changed turning a value that was valid when written
    into an out of bounds value.

    While here, add further restrictions to the head register (TDH).  The
    manual states that writing to this value while transmit is enabled can
    cause unexpected behavior and that it should only be written after a
    reset.  As such, ignore attempts to write while transmit is active,
    and also ignore writes of non-zero values.  Later e1000 chipsets have
    this register as read-only.

    Also ignore any attempts to transmit packets if the transmit ring's
    size is zero.

    PR:             264567
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Reviewed by:    emaste
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D36269

    (cherry picked from commit 7afe342dcb38b624488009bb6bdfa5337e628ffc)

 usr.sbin/bhyve/pci_e82545.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
Comment 6 Ed Maste freebsd_committer freebsd_triage 2023-01-20 18:35:47 UTC
Committed and merged to stable branches