bhyve: simplify slot validation in xHCI emulation

This is a follow-up to commit e72d86ad9c62 ("bhyve: improve input
validation in pci_xhci") -- introducing a helper for slot validation.

Co-authored

bhyve: simplify slot validation in xHCI emulation

This is a follow-up to commit e72d86ad9c62 ("bhyve: improve input
validation in pci_xhci") -- introducing a helper for slot validation.

Co-authored-by: John Baldwin <jhb@FreeBSD.org>
Reviewed by: markj, emaste
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D46696

show more ...

bhyve: improve input validation in pci_xhci

Several functions did not validate the slot index resulting in OOB read
on the heap of the slot device structure which could lead to arbitrary
reads/write

bhyve: improve input validation in pci_xhci

Several functions did not validate the slot index resulting in OOB read
on the heap of the slot device structure which could lead to arbitrary
reads/writes and potentially code execution.

Reported by: Synacktiv
Reviewed by: markj (earlier), jhb
Security: CVE-2024-41721
Security: HYP-02
Sponsored by: The Alpha-Omega Project
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D45996

show more ...

bhyve: fix off by one error in pci_xhci

The function pci_xhci_find_stream validates that the streamid is valid
but the bound check accepts up to ep_MaxPStreams included.

The bug results in an out-o

bhyve: fix off by one error in pci_xhci

The function pci_xhci_find_stream validates that the streamid is valid
but the bound check accepts up to ep_MaxPStreams included.

The bug results in an out-of-bounds write on the heap with controlled
data.

Reported by: Synacktiv
Reviewed by: jhb
Security: FreeBSD-SA-24:12.bhyve
Security: CVE-2024-32668
Security: HYP-04
Sponsored by: The Alpha-Omega Project
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D45994

show more ...

usr.sbin: Automated cleanup of cdefs and other formatting

Apply the following automated changes to try to eliminate
no-longer-needed sys/cdefs.h includes as well as now-empty
blank lines in a row.

usr.sbin: Automated cleanup of cdefs and other formatting

Apply the following automated changes to try to eliminate
no-longer-needed sys/cdefs.h includes as well as now-empty
blank lines in a row.

Remove /^#if.*\n#endif.*\n#include\s+<sys/cdefs.h>.*\n/
Remove /\n+#include\s+<sys/cdefs.h>.*\n+#if.*\n#endif.*\n+/
Remove /\n+#if.*\n#endif.*\n+/
Remove /^#if.*\n#endif.*\n/
Remove /\n+#include\s+<sys/cdefs.h>\n#include\s+<sys/types.h>/
Remove /\n+#include\s+<sys/cdefs.h>\n#include\s+<sys/param.h>/
Remove /\n+#include\s+<sys/cdefs.h>\n#include\s+<sys/capsicum.h>/

Sponsored by: Netflix

show more ...

bhyve: Replace many fprintf(stderr, ...) calls with EPRINTLN

EPRINTLN handles newlines appropriately when stdout/stderr have been
reused as the backend for a serial port.

For bhyverun.c itself, the

bhyve: Replace many fprintf(stderr, ...) calls with EPRINTLN

EPRINTLN handles newlines appropriately when stdout/stderr have been
reused as the backend for a serial port.

For bhyverun.c itself, the rule this attempts to follow is to use
regular fprintf/perror/warn/err prior to init_pci() (which is when
serial ports are configured) and to switch to EPRINTLN afterwards.

Reviewed by: corvink, markj
Differential Revision: https://reviews.freebsd.org/D42182

show more ...

Remove $FreeBSD$: one-line .c pattern

Remove /^[\s*]*__FBSDID\("\$FreeBSD\$"\);?\s*\n/

spdx: The BSD-2-Clause-FreeBSD identifier is obsolete, drop -FreeBSD

The SPDX folks have obsoleted the BSD-2-Clause-FreeBSD identifier. Catch
up to that fact and revert to their recommended match of

spdx: The BSD-2-Clause-FreeBSD identifier is obsolete, drop -FreeBSD

The SPDX folks have obsoleted the BSD-2-Clause-FreeBSD identifier. Catch
up to that fact and revert to their recommended match of BSD-2-Clause.

Discussed with: pfg
MFC After: 3 days
Sponsored by: Netflix

show more ...

bhyve: Remove vmctx member from struct vm_snapshot_meta.

This is a userland-only pointer that isn't relevant to the kernel and
doesn't belong in the ioctl structure shared between userland and the
k

bhyve: Remove vmctx member from struct vm_snapshot_meta.

This is a userland-only pointer that isn't relevant to the kernel and
doesn't belong in the ioctl structure shared between userland and the
kernel. For the kernel, the old structure for the ioctl is still
supported under COMPAT_FREEBSD13.

This changes vm_snapshot_req() in libvmmapi to accept an explicit
vmctx argument.

It also changes vm_snapshot_guest2host_addr to take an explicit vmctx
argument. As part of this change, move the declaration for this
function and its wrapper macro from vmm_snapshot.h to snapshot.h as it
is a userland-only API.

Reviewed by: corvink, markj
Differential Revision: https://reviews.freebsd.org/D38125

show more ...

bhyve: Remove vmctx argument from PCI device model methods.

Most of these arguments were unused. Device models which do need
access to the vmctx in one of these methods can obtain it from the
pi_vm

bhyve: Remove vmctx argument from PCI device model methods.

Most of these arguments were unused. Device models which do need
access to the vmctx in one of these methods can obtain it from the
pi_vmctx member of the pci_devinst argument instead.

Reviewed by: corvink, markj
Differential Revision: https://reviews.freebsd.org/D38096

show more ...

bhyve: Avoid triggering false -Wfree-nonheap-object warnings.

XHCI port and slot numbers are 1-based rather than 0-based. To handle
this, bhyve was subtracting one item from the pointers saved in t

bhyve: Avoid triggering false -Wfree-nonheap-object warnings.

XHCI port and slot numbers are 1-based rather than 0-based. To handle
this, bhyve was subtracting one item from the pointers saved in the
softc so that index 1 accessed index 0 of the allocated array.

However, this is UB and confused GCC 12. The compiler noticed that
the calls to free() were using an offset and emitted a warning.
Rather than storing UB pointers in the softc, push the decrement
operation into the existing macros that wrap accesses to the relevant
arrays.

Reviewed by: corvink, markj
Differential Revision: https://reviews.freebsd.org/D36829

show more ...

bhyve: Remove unused vcpu argument from PCI read/write methods.

Reviewed by: corvink, markj
Differential Revision: https://reviews.freebsd.org/D37652

bhyve: Avoid using a packed struct for xhci port registers

I believe the __packed annotation is there only because
pci_xhci_portregs_read() is treating the register set as an array of
uint32_t. cla

bhyve: Avoid using a packed struct for xhci port registers

I believe the __packed annotation is there only because
pci_xhci_portregs_read() is treating the register set as an array of
uint32_t. clang warns about taking the address of portregs->portsc
because it is a packed member and thus might not have expected
alignment.

Fix the problem by simply selecting the field to read with a switch
statement. This mimics pci_xhci_portregs_write(). While here, switch
to using some symbolic constants.

There is a small semantic change here in that pci_xhci_portregs_read()
would silently truncate unaligned offsets. For consistency with
pci_xhci_portregs_write(), which does not do that, return all ones for
unaligned reads instead.

MFC after: 2 weeks
Reviewed by: corvink, jhb
Differential Revision: https://reviews.freebsd.org/D37408

show more ...

bhyve: Use XHCI_PORTREG_PTR in one place that open-coded it.

Reviewed by: corvink, markj
Differential Revision: https://reviews.freebsd.org/D36888

bhyve: Simplify control flow in the xhci device model

We only need to call pci_xhci_xfer_complete() when handling a transfer
to the control endpoint, so move that code into the epid == 1 block and
e

bhyve: Simplify control flow in the xhci device model

We only need to call pci_xhci_xfer_complete() when handling a transfer
to the control endpoint, so move that code into the epid == 1 block and
eliminate a goto. Also remove an unneeded reinitialization of
setup_trb.

No functional change intended.

MFC after: 1 week
Reviewed by: corvink, jhb
Differential Revision: https://reviews.freebsd.org/D37287

show more ...

bhyve: Fix an apparent pointer arithmetic bug in the xhci emulation

Also remove the out-parameter of pci_xhci_find_stream(), since it's
unused by all callers.

MFC after: 1 week
Reviewed by: jhb
Dif

bhyve: Fix an apparent pointer arithmetic bug in the xhci emulation

Also remove the out-parameter of pci_xhci_find_stream(), since it's
unused by all callers.

MFC after: 1 week
Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D37118

show more ...

bhyve: Address some signed/unsigned comparison warnings

MFC after: 1 week

bhyve: Annotate unused function parameters

MFC after: 1 week

bhyve xhci: Cache the value of MaxPStreams when initializing an endpoint.

This avoids type confusion where a malicious guest could rewrite the
MaxPStreams field in an endpoint context after the endp

bhyve xhci: Cache the value of MaxPStreams when initializing an endpoint.

This avoids type confusion where a malicious guest could rewrite the
MaxPStreams field in an endpoint context after the endpoint was
initialized causing the device model to interpret a guest provided
address (stored in ep_ringaddr of the "software" endpoint state) as a
bhyve host process address (ep_sctx_trbs). It also prevents a malicious
guest from triggering overflows of ep_sctx_trbs[] by increasing the
number of streams after the endpoint has been initialized.

Rather than re-reading the MaxPStreams value out of the endpoint context
in guest memory on subsequent operations, cache the value in the software
endpoint state. Possibly the device model should raise errors if the
value of MaxPStreams changes while an endpoint is running. This approach
simply ignores any such changes by the guest.

PR: 264294, 264347
Reported by: Robert Morris <rtm@lcs.mit.edu>
Reviewed by: markj
MFC after: 1 week
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D36181

show more ...

bhyve: Mark variables and functions as static where appropriate

Mark them const as well when it makes sense to do so. No functional
change intended.

MFC after: 1 week
Sponsored by: The FreeBSD Fou

bhyve: Mark variables and functions as static where appropriate

Mark them const as well when it makes sense to do so. No functional
change intended.

MFC after: 1 week
Sponsored by: The FreeBSD Foundation

show more ...

bhyve: clean up trailing whitespaces

Clean up trailing whitespaces. No functional changes.

Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D33681

Fix typo in xhci nvlist node name, and also increment device counter.

This allows the xhci tablet device to be recognized and a PCI device
instantiated.

Reviewed by: jhb
Fixes: 621b5090487d Refact

Fix typo in xhci nvlist node name, and also increment device counter.

This allows the xhci tablet device to be recognized and a PCI device
instantiated.

Reviewed by: jhb
Fixes: 621b5090487d Refactor configuration management in bhyve.
MFC after: 3 months.

show more ...

Refactor configuration management in bhyve.

Replace the existing ad-hoc configuration via various global variables
with a small database of key-value pairs. The database supports
heirarchical keys

Refactor configuration management in bhyve.

Replace the existing ad-hoc configuration via various global variables
with a small database of key-value pairs. The database supports
heirarchical keys using a MIB-like syntax to name the path to a given
key. Values are always stored as strings. The API used to manage
configuation values does include wrappers to handling boolean values.
Other values use non-string types require parsing by consumers.

The configuration values are stored in a tree using nvlists. Leaf
nodes hold string values. Configuration values are permitted to
reference other configuration values using '%(name)'. This permits
constructing template configurations.

All existing command line arguments now set configuration values. For
devices, the "-s" option parses its option argument to generate a list
of key-value pairs for the given device.

A new '-o' command line option permits setting an individual
configuration variable. The key name is always given as a full path
of dot-separated components.

A new '-k' command line option parses a simple configuration file.
This configuration file holds a flat list of 'key=value' lines where
the 'key' is the full path of a configuration variable. Lines
starting with a '#' are comments.

In general, bhyve starts by parsing command line options in sequence
and applying those settings to configuration values. Once this is
complete, bhyve then begins initializing its state based on the
configuration values. This means that subsequent configuration
options or files may override or supplement previously given settings.

A special 'config.dump' configuration value can be set to true to help
debug configuration issues. When this value is set, bhyve will print
out the configuration variables as a flat list of 'key=value' lines.

Most command line argments map to a single configuration variable,
e.g. '-w' sets the 'x86.strictmsr' value to false. A few command
line arguments have less obvious effects:

- Multiple '-p' options append their values (as a comma-seperated
list) to "vcpu.N.cpuset" values (where N is a decimal vcpu number).

- For '-s' options, a pci.<bus>.<slot>.<function> node is created.
The first argument to '-s' (the device type) is used as the value of
a "device" variable. Additional comma-separated arguments are then
parsed into 'key=value' pairs and used to set additional variables
under the device node. A PCI device emulation driver can provide
its own hook to override the parsing of the additonal '-s' arguments
after the device type.

After the configuration phase as completed, the init_pci hook
then walks the "pci.<bus>.<slot>.<func>" nodes. It uses the
"device" value to find the device model to use. The device
model's init routine is passed a reference to its nvlist node
in the configuration tree which it can query for specific
variables.

The result is that a lot of the string parsing is removed from
the device models and centralized. In addition, adding a new
variable just requires teaching the model to look for the new
variable.

- For '-l' options, a similar model is used where the string is
parsed into values that are later read during initialization.
One key note here is that the serial ports use the commonly
used lowercase names from existing documentation and examples
(e.g. "lpc.com1") instead of the uppercase names previously
used internally in bhyve.

Reviewed by: grehan
MFC after: 3 months
Differential Revision: https://reviews.freebsd.org/D26035

show more ...

bhyve: 'xhci,tablet' snapshot fixes

Permit suspend/resume of a XHCI device model that has not been
attached to by a driver in a guest OS.

Submitted by: Vitaliy Gusev <gusev.vitaliy@gmail.com>
Diffe

bhyve: 'xhci,tablet' snapshot fixes

Permit suspend/resume of a XHCI device model that has not been
attached to by a driver in a guest OS.

Submitted by: Vitaliy Gusev <gusev.vitaliy@gmail.com>
Differential Revision: https://reviews.freebsd.org/D26264

show more ...

Advertise 64-bit physical-address capability.

This fixes a coredump with NetBSD guests when XHCI is configured.
On seeing the AC64 flag clear, the NetBSD XHCI driver was only writing
to the lower 32

Advertise 64-bit physical-address capability.

This fixes a coredump with NetBSD guests when XHCI is configured.
On seeing the AC64 flag clear, the NetBSD XHCI driver was only writing
to the lower 32-bits of 64-bit physical address registers. The emulation
relies on a write to the hi 32-bits to calculate a host virtual address
for internal use, and has always supported 64-bit addressing.

All other guests were seen to write to both the lo- and hi- address
registers, regardless of the AC64 setting.

Discussed with: Leon Dang (author)
Tested with: Ubuntu 16/18/20, Windows10, OpenBSD UEFI guests.

MFC after: 2 weeks.

show more ...

Prevent calling USB backends multiple times.

The TRB processing loop could potentially call a back-end twice
with the same status transaction. While this was generally benign,
some code paths in the

Prevent calling USB backends multiple times.

The TRB processing loop could potentially call a back-end twice
with the same status transaction. While this was generally benign,
some code paths in the tablet backend weren't set up to handle
this case, resulting in a NULL dereference.

Fix by
- returning a STALL error when an invalid request was seen in the backend
- skipping a call to the backend if the number of packets in a status
transaction was zero (this code fragment was taken from the Intel ACRN
xhci backend)

PR: 246964
Reported by: Ali Abdallah
Discussed with: Leon Dang (author)
Reviewed by: jhb (#bhyve), Leon Dang
Approved by: jhb
Obtained from: Intel ACRN (partially)
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D25228

show more ...