Page MenuHomeFreeBSD

bhyve(8): Fix NVMe data structure copy to guest
ClosedPublic

Authored by chuck on Mar 24 2019, 5:18 PM.

Details

Summary

bhyve's NVMe emulation was transferring Identify data back to the guest
incorrectly causing memory corruptions. These corruptions resulted in
core dumps and other system level errors in the guest.

In their simplest form, NVMe Physical Region Page (PRP) values in
commands indicate which physical pages to use for data transfer. The
first PRP value is not required to be page aligned but does not cross a
page boundary. The second PRP value must be page aligned, does not cross
a page boundary, and need not be contiguous with PRP1.

The code was copying Identify data past the end of PRP1. This happens to
work if PRP1 and PRP2 are physically contiguous but will corrupt guest
memory in unpredictable ways if they are not.

Fix is to copy the Identify data back to the guest piecewise (i.e. for
each PRP entry). Also fix a similarly wrong problem when copying back
Log page data.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chuck created this revision.Mar 24 2019, 5:18 PM
imp accepted this revision.Mar 31 2019, 4:14 PM

This looks good to me. It's the right thing to do.
Do we need to expand it more generally (in a different commit) for any time we have to copy data out, or is the I/O path good?

araujo accepted this revision.Apr 3 2019, 3:29 AM

It looks good! Thanks again @chuck

rgrimes accepted this revision as: rgrimes.Apr 3 2019, 9:01 AM

Confirmation of a working Windows 2019 boot after this patch: https://lists.freebsd.org/pipermail/freebsd-virtualization/2019-April/007333.html

jhb accepted this revision.Apr 4 2019, 5:23 PM
This revision is now accepted and ready to land.Apr 4 2019, 5:23 PM
chuck added a comment.Apr 5 2019, 4:00 PM
In D19695#423813, @imp wrote:

This looks good to me. It's the right thing to do.
Do we need to expand it more generally (in a different commit) for any time we have to copy data out, or is the I/O path good?

The IO path appears to be OK, but I only took brief look. That said, I've run a fair amount of IO against and haven't seen any issues in practice. Where as this issue was easily reproducible.

imp added a comment.Apr 5 2019, 4:20 PM
In D19695#423813, @imp wrote:

This looks good to me. It's the right thing to do.
Do we need to expand it more generally (in a different commit) for any time we have to copy data out, or is the I/O path good?

The IO path appears to be OK, but I only took brief look. That said, I've run a fair amount of IO against and haven't seen any issues in practice. Where as this issue was easily reproducible.

Good enough for me. Let's get it in.

This revision was automatically updated to reflect the committed changes.