| Message ID | 20181116093152.27227-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-3.1] nvme: fix out-of-bounds access to the CMB | expand |
Paolo Bonzini <pbonzini@redhat.com> 于2018年11月16日周五 下午5:31写道： > Because the CMB BAR has a min_access_size of 2, if you read the last > byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one > error. This is CVE-2018-16847. > > Another way to fix this might be to register the CMB as a RAM memory > region, which would also be more efficient. However, that might be a > change for big-endian machines; I didn't think this through and I don't > know how real hardware works. Add a basic testcase for the CMB in case > somebody does this change later on. > > Cc: Keith Busch <keith.busch@intel.com> > Cc: qemu-block@nongnu.org > Cc: Li Qiang <liq3ea@gmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- > hw/block/nvme.c | 2 +- > tests/Makefile.include | 2 +- > tests/nvme-test.c | 58 +++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 57 insertions(+), 5 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 09d7c90259..5d92794ef7 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = { > .write = nvme_cmb_write, > .endianness = DEVICE_LITTLE_ENDIAN, > .impl = { > - .min_access_size = 2, > + .min_access_size = 1, > .max_access_size = 8, > }, > }; > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 613242bc6e..fb0b449c02 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o > tests/machine-none-test$(EXESUF): tests/machine-none-test.o > tests/drive_del-test$(EXESUF): tests/drive_del-test.o > $(libqos-virtio-obj-y) > tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o > $(libqos-pc-obj-y) > -tests/nvme-test$(EXESUF): tests/nvme-test.o > +tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y) > tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o > tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o > tests/ac97-test$(EXESUF): tests/ac97-test.o > diff --git a/tests/nvme-test.c b/tests/nvme-test.c > index 7674a446e4..2abb3b6d19 100644 > --- a/tests/nvme-test.c > +++ b/tests/nvme-test.c > @@ -8,11 +8,64 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "libqtest.h" > +#include "libqos/libqos-pc.h" > + > +static QOSState *qnvme_start(const char *extra_opts) > +{ > + QOSState *qs; > + const char *arch = qtest_get_arch(); > + const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw " > + "-device nvme,addr=0x4.0,serial=foo,drive=drv0 %s"; > + > + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { > + qs = qtest_pc_boot(cmd, extra_opts ? : ""); > + global_qtest = qs->qts; > + return qs; > + } > + > + g_printerr("nvme tests are only available on x86\n"); > + exit(EXIT_FAILURE); > +} > + > +static void qnvme_stop(QOSState *qs) > +{ > + qtest_shutdown(qs); > +} > > -/* Tests only initialization so far. TODO: Replace with functional tests > */ > static void nop(void) > { > + QOSState *qs; > + > + qs = qnvme_start(NULL); > + qnvme_stop(qs); > +} > + > +static void nvmetest_cmb_test(void) > +{ > + const int cmb_bar_size = 2 * MiB; > + QOSState *qs; > + QPCIDevice *pdev; > + QPCIBar bar; > + > + qs = qnvme_start("-global nvme.cmb_size_mb=2"); > + pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0)); > + g_assert(pdev != NULL); > + > + qpci_device_enable(pdev); > + bar = qpci_iomap(pdev, 2, NULL); > + > + qpci_io_writel(pdev, bar, 0, 0xccbbaa99); > + g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99); > + g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99); > + > + /* Test partially out-of-bounds accesses. */ > + qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211); > + g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11); > + g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, > 0x2211); > + g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, > 0x44332211); > Here seems need a g_free(pdev), > + qnvme_stop(qs); > } > > int main(int argc, char **argv) > @@ -21,9 +74,8 @@ int main(int argc, char **argv) > > g_test_init(&argc, &argv, NULL); > qtest_add_func("/nvme/nop", nop); > + qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test); > > - qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw " > - "-device nvme,drive=drv0,serial=foo"); > ret = g_test_run(); > > qtest_end(); > There is no qtest_start(), this qtest_end() seems trigger an assert in glib. (tests/nvme-test:44053): GLib-CRITICAL **: g_hook_destroy_link: assertion 'hook != NULL' failed Otherwise Reviewed-by: Li Qiang <liq3ea@gmail.com> Tested-by: Li Qiang <liq3ea@gmail.com> Thanks, Li Qiang > -- > 2.19.1 > >
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20181116093152.27227-1-pbonzini@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3ab66e7 nvme: fix out-of-bounds access to the CMB
=== OUTPUT BEGIN ===
Checking PATCH 1/1: nvme: fix out-of-bounds access to the CMB...
ERROR: space required after that ',' (ctx:VxV)
#99: FILE: tests/nvme-test.c:53:
+ pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0));
^
total: 1 errors, 0 warnings, 91 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as opposed to this one). Was this done in error? Thanks, -Mark On 11/16/2018 3:31 AM, Paolo Bonzini wrote: > Because the CMB BAR has a min_access_size of 2, if you read the last > byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one > error. This is CVE-2018-16847. > > Another way to fix this might be to register the CMB as a RAM memory > region, which would also be more efficient. However, that might be a > change for big-endian machines; I didn't think this through and I don't > know how real hardware works. Add a basic testcase for the CMB in case > somebody does this change later on. > > Cc: Keith Busch <keith.busch@intel.com> > Cc: qemu-block@nongnu.org > Cc: Li Qiang <liq3ea@gmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/block/nvme.c | 2 +- > tests/Makefile.include | 2 +- > tests/nvme-test.c | 58 +++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 57 insertions(+), 5 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 09d7c90259..5d92794ef7 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = { > .write = nvme_cmb_write, > .endianness = DEVICE_LITTLE_ENDIAN, > .impl = { > - .min_access_size = 2, > + .min_access_size = 1, > .max_access_size = 8, > }, > }; > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 613242bc6e..fb0b449c02 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o > tests/machine-none-test$(EXESUF): tests/machine-none-test.o > tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-virtio-obj-y) > tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y) > -tests/nvme-test$(EXESUF): tests/nvme-test.o > +tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y) > tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o > tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o > tests/ac97-test$(EXESUF): tests/ac97-test.o > diff --git a/tests/nvme-test.c b/tests/nvme-test.c > index 7674a446e4..2abb3b6d19 100644 > --- a/tests/nvme-test.c > +++ b/tests/nvme-test.c > @@ -8,11 +8,64 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/units.h" > #include "libqtest.h" > +#include "libqos/libqos-pc.h" > + > +static QOSState *qnvme_start(const char *extra_opts) > +{ > + QOSState *qs; > + const char *arch = qtest_get_arch(); > + const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw " > + "-device nvme,addr=0x4.0,serial=foo,drive=drv0 %s"; > + > + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { > + qs = qtest_pc_boot(cmd, extra_opts ? : ""); > + global_qtest = qs->qts; > + return qs; > + } > + > + g_printerr("nvme tests are only available on x86\n"); > + exit(EXIT_FAILURE); > +} > + > +static void qnvme_stop(QOSState *qs) > +{ > + qtest_shutdown(qs); > +} > > -/* Tests only initialization so far. TODO: Replace with functional tests */ > static void nop(void) > { > + QOSState *qs; > + > + qs = qnvme_start(NULL); > + qnvme_stop(qs); > +} > + > +static void nvmetest_cmb_test(void) > +{ > + const int cmb_bar_size = 2 * MiB; > + QOSState *qs; > + QPCIDevice *pdev; > + QPCIBar bar; > + > + qs = qnvme_start("-global nvme.cmb_size_mb=2"); > + pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0)); > + g_assert(pdev != NULL); > + > + qpci_device_enable(pdev); > + bar = qpci_iomap(pdev, 2, NULL); > + > + qpci_io_writel(pdev, bar, 0, 0xccbbaa99); > + g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99); > + g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99); > + > + /* Test partially out-of-bounds accesses. */ > + qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211); > + g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11); > + g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, 0x2211); > + g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 0x44332211); > + qnvme_stop(qs); > } > > int main(int argc, char **argv) > @@ -21,9 +74,8 @@ int main(int argc, char **argv) > > g_test_init(&argc, &argv, NULL); > qtest_add_func("/nvme/nop", nop); > + qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test); > > - qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw " > - "-device nvme,drive=drv0,serial=foo"); > ret = g_test_run(); > > qtest_end(); >
On 19/11/18 16:23, Mark Kanda wrote: > For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as > opposed to this one). Was this done in error? Probably. Kevin, can you revert and apply this one instead? I don't care if 3.1 or 3.2, but the previous fix is pointless complication. Paolo > > Thanks, > > -Mark > > On 11/16/2018 3:31 AM, Paolo Bonzini wrote: >> Because the CMB BAR has a min_access_size of 2, if you read the last >> byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one >> error. This is CVE-2018-16847. >> >> Another way to fix this might be to register the CMB as a RAM memory >> region, which would also be more efficient. However, that might be a >> change for big-endian machines; I didn't think this through and I don't >> know how real hardware works. Add a basic testcase for the CMB in case >> somebody does this change later on. >> >> Cc: Keith Busch <keith.busch@intel.com> >> Cc: qemu-block@nongnu.org >> Cc: Li Qiang <liq3ea@gmail.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/block/nvme.c | 2 +- >> tests/Makefile.include | 2 +- >> tests/nvme-test.c | 58 +++++++++++++++++++++++++++++++++++++++--- >> 3 files changed, 57 insertions(+), 5 deletions(-) >> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c >> index 09d7c90259..5d92794ef7 100644 >> --- a/hw/block/nvme.c >> +++ b/hw/block/nvme.c >> @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = { >> .write = nvme_cmb_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> .impl = { >> - .min_access_size = 2, >> + .min_access_size = 1, >> .max_access_size = 8, >> }, >> }; >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index 613242bc6e..fb0b449c02 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o >> tests/machine-none-test$(EXESUF): tests/machine-none-test.o >> tests/drive_del-test$(EXESUF): tests/drive_del-test.o >> $(libqos-virtio-obj-y) >> tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o >> $(libqos-pc-obj-y) >> -tests/nvme-test$(EXESUF): tests/nvme-test.o >> +tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y) >> tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o >> tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o >> tests/ac97-test$(EXESUF): tests/ac97-test.o >> diff --git a/tests/nvme-test.c b/tests/nvme-test.c >> index 7674a446e4..2abb3b6d19 100644 >> --- a/tests/nvme-test.c >> +++ b/tests/nvme-test.c >> @@ -8,11 +8,64 @@ >> */ >> #include "qemu/osdep.h" >> +#include "qemu/units.h" >> #include "libqtest.h" >> +#include "libqos/libqos-pc.h" >> + >> +static QOSState *qnvme_start(const char *extra_opts) >> +{ >> + QOSState *qs; >> + const char *arch = qtest_get_arch(); >> + const char *cmd = "-drive >> id=drv0,if=none,file=null-co://,format=raw " >> + "-device nvme,addr=0x4.0,serial=foo,drive=drv0 >> %s"; >> + >> + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { >> + qs = qtest_pc_boot(cmd, extra_opts ? : ""); >> + global_qtest = qs->qts; >> + return qs; >> + } >> + >> + g_printerr("nvme tests are only available on x86\n"); >> + exit(EXIT_FAILURE); >> +} >> + >> +static void qnvme_stop(QOSState *qs) >> +{ >> + qtest_shutdown(qs); >> +} >> -/* Tests only initialization so far. TODO: Replace with functional >> tests */ >> static void nop(void) >> { >> + QOSState *qs; >> + >> + qs = qnvme_start(NULL); >> + qnvme_stop(qs); >> +} >> + >> +static void nvmetest_cmb_test(void) >> +{ >> + const int cmb_bar_size = 2 * MiB; >> + QOSState *qs; >> + QPCIDevice *pdev; >> + QPCIBar bar; >> + >> + qs = qnvme_start("-global nvme.cmb_size_mb=2"); >> + pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0)); >> + g_assert(pdev != NULL); >> + >> + qpci_device_enable(pdev); >> + bar = qpci_iomap(pdev, 2, NULL); >> + >> + qpci_io_writel(pdev, bar, 0, 0xccbbaa99); >> + g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99); >> + g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99); >> + >> + /* Test partially out-of-bounds accesses. */ >> + qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211); >> + g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, >> 0x11); >> + g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, >> 0x2211); >> + g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, >> 0x44332211); >> + qnvme_stop(qs); >> } >> int main(int argc, char **argv) >> @@ -21,9 +74,8 @@ int main(int argc, char **argv) >> g_test_init(&argc, &argv, NULL); >> qtest_add_func("/nvme/nop", nop); >> + qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test); >> - qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw " >> - "-device nvme,drive=drv0,serial=foo"); >> ret = g_test_run(); >> qtest_end(); >>
Am 19.11.2018 um 18:09 hat Paolo Bonzini geschrieben: > On 19/11/18 16:23, Mark Kanda wrote: > > For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as > > opposed to this one). Was this done in error? > > Probably. Kevin, can you revert and apply this one instead? I don't > care if 3.1 or 3.2, but the previous fix is pointless complication. I was waiting for you to address Li Qiang's review comments before I apply it. I can revert the other one once this is ready. > > On 11/16/2018 3:31 AM, Paolo Bonzini wrote: > >> Because the CMB BAR has a min_access_size of 2, if you read the last > >> byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one > >> error. This is CVE-2018-16847. > >> > >> Another way to fix this might be to register the CMB as a RAM memory > >> region, which would also be more efficient. However, that might be a > >> change for big-endian machines; I didn't think this through and I don't > >> know how real hardware works. Add a basic testcase for the CMB in case > >> somebody does this change later on. > >> > >> Cc: Keith Busch <keith.busch@intel.com> > >> Cc: qemu-block@nongnu.org > >> Cc: Li Qiang <liq3ea@gmail.com> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> --- > >> hw/block/nvme.c | 2 +- > >> tests/Makefile.include | 2 +- > >> tests/nvme-test.c | 58 +++++++++++++++++++++++++++++++++++++++--- > >> 3 files changed, 57 insertions(+), 5 deletions(-) > >> > >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c > >> index 09d7c90259..5d92794ef7 100644 > >> --- a/hw/block/nvme.c > >> +++ b/hw/block/nvme.c > >> @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = { > >> .write = nvme_cmb_write, > >> .endianness = DEVICE_LITTLE_ENDIAN, > >> .impl = { > >> - .min_access_size = 2, > >> + .min_access_size = 1, > >> .max_access_size = 8, > >> }, > >> }; Anyway, that .min_access_size influences the accessible range feels weird to me. Is this really how it is meant to work? I expected this only to influence the allowed granularity of accesses, and that the maximum accessible offset of the memory region is size - access_size. Does this mean that the size parameter of memory_region_init_io() really means we allow access to offsets from 0 to size + impl.min_access_size - 1? If so, this is very surprising and I wonder if this is really the only device that gets it wrong. For nvme it doesn't matter much because it can trivially support single-byte accesses, so this change is correct and fixes the problem, but isn't the real bug in access_with_adjusted_size(), which should adjust the accessed range in a way that it doesn't exceed the size of the memory region? I'm not sure why impl.min_access_size was set to 2 in the first place, but was valid.min_access_size meant maybe? Though if I read the spec correctly, that one should be 4, not 2. Hm... But memory_region_access_valid() doesn't even check min and max access size if an accepts function pointer isn't given as well. Yet, there are devices that set min/max, but not accepts. Am I missing where this actually takes effect or are they buggy? This stuff really confuses me. Kevin
On 19/11/18 18:43, Kevin Wolf wrote: > Am 19.11.2018 um 18:09 hat Paolo Bonzini geschrieben: >> On 19/11/18 16:23, Mark Kanda wrote: >>> For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as >>> opposed to this one). Was this done in error? >> >> Probably. Kevin, can you revert and apply this one instead? I don't >> care if 3.1 or 3.2, but the previous fix is pointless complication. > > I was waiting for you to address Li Qiang's review comments before I > apply it. I can revert the other one once this is ready. Sorry, I forgot to send it. Did it now. > Anyway, that .min_access_size influences the accessible range feels > weird to me. Is this really how it is meant to work? I expected this > only to influence the allowed granularity of accesses, and that the > maximum accessible offset of the memory region is size - access_size. >> Does this mean that the size parameter of memory_region_init_io() really > means we allow access to offsets from 0 to size + impl.min_access_size - 1? > If so, this is very surprising and I wonder if this is really the only > device that gets it wrong. Usually the offset is a register, so an invalid value will simply be ignored by the device or reported as a guest error. > For nvme it doesn't matter much because it can trivially support > single-byte accesses, so this change is correct and fixes the problem, > but isn't the real bug in access_with_adjusted_size(), which should > adjust the accessed range in a way that it doesn't exceed the size of > the memory region? Hmm, what's happening is complicated. memory_access_size is clamping the access size to 1 because impl.unaligned is false. However, access_with_adjusted_size is bringing it back to 2 because it does access_size = MAX(MIN(size, access_size_max), access_size_min); So we could do something like diff --git a/exec.c b/exec.c index bb6170dbff..f1437b2be6 100644 --- a/exec.c +++ b/exec.c @@ -3175,7 +3175,11 @@ if (!mr->ops->impl.unaligned) { unsigned align_size_max = addr & -addr; if (align_size_max != 0 && align_size_max < access_size_max) { - access_size_max = align_size_max; + unsigned access_size_min = mr->ops->valid.min_access_size; + if (access_size_min == 0) { + access_size_min = 1; + } + access_size_max = MAX(min_access_size, align_size_max); } } Then I think the access size would remain 2 and and memory_region_access_valid would reject it as unaligned. That would avoid the bug, but then nvme should be setting valid.min_access_size and the exec.c patch alone would not be enough. > I'm not sure why impl.min_access_size was set to 2 in the first place, > but was valid.min_access_size meant maybe? Though if I read the spec > correctly, that one should be 4, not 2. I don't see any requirement for the CMB (section 4.7 in my copy)? Paolo
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 09d7c90259..5d92794ef7 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = { .write = nvme_cmb_write, .endianness = DEVICE_LITTLE_ENDIAN, .impl = { - .min_access_size = 2, + .min_access_size = 1, .max_access_size = 8, }, }; diff --git a/tests/Makefile.include b/tests/Makefile.include index 613242bc6e..fb0b449c02 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o tests/machine-none-test$(EXESUF): tests/machine-none-test.o tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-virtio-obj-y) tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y) -tests/nvme-test$(EXESUF): tests/nvme-test.o +tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y) tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o tests/ac97-test$(EXESUF): tests/ac97-test.o diff --git a/tests/nvme-test.c b/tests/nvme-test.c index 7674a446e4..2abb3b6d19 100644 --- a/tests/nvme-test.c +++ b/tests/nvme-test.c @@ -8,11 +8,64 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "libqtest.h" +#include "libqos/libqos-pc.h" + +static QOSState *qnvme_start(const char *extra_opts) +{ + QOSState *qs; + const char *arch = qtest_get_arch(); + const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw " + "-device nvme,addr=0x4.0,serial=foo,drive=drv0 %s"; + + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { + qs = qtest_pc_boot(cmd, extra_opts ? : ""); + global_qtest = qs->qts; + return qs; + } + + g_printerr("nvme tests are only available on x86\n"); + exit(EXIT_FAILURE); +} + +static void qnvme_stop(QOSState *qs) +{ + qtest_shutdown(qs); +} -/* Tests only initialization so far. TODO: Replace with functional tests */ static void nop(void) { + QOSState *qs; + + qs = qnvme_start(NULL); + qnvme_stop(qs); +} + +static void nvmetest_cmb_test(void) +{ + const int cmb_bar_size = 2 * MiB; + QOSState *qs; + QPCIDevice *pdev; + QPCIBar bar; + + qs = qnvme_start("-global nvme.cmb_size_mb=2"); + pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0)); + g_assert(pdev != NULL); + + qpci_device_enable(pdev); + bar = qpci_iomap(pdev, 2, NULL); + + qpci_io_writel(pdev, bar, 0, 0xccbbaa99); + g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99); + g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99); + + /* Test partially out-of-bounds accesses. */ + qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211); + g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11); + g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=, 0x2211); + g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 0x44332211); + qnvme_stop(qs); } int main(int argc, char **argv) @@ -21,9 +74,8 @@ int main(int argc, char **argv) g_test_init(&argc, &argv, NULL); qtest_add_func("/nvme/nop", nop); + qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test); - qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw " - "-device nvme,drive=drv0,serial=foo"); ret = g_test_run(); qtest_end();
Because the CMB BAR has a min_access_size of 2, if you read the last byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one error. This is CVE-2018-16847. Another way to fix this might be to register the CMB as a RAM memory region, which would also be more efficient. However, that might be a change for big-endian machines; I didn't think this through and I don't know how real hardware works. Add a basic testcase for the CMB in case somebody does this change later on. Cc: Keith Busch <keith.busch@intel.com> Cc: qemu-block@nongnu.org Cc: Li Qiang <liq3ea@gmail.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/block/nvme.c | 2 +- tests/Makefile.include | 2 +- tests/nvme-test.c | 58 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 57 insertions(+), 5 deletions(-)