* [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
@ 2018-11-16 9:31 Paolo Bonzini
2018-11-16 10:38 ` Li Qiang
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Paolo Bonzini @ 2018-11-16 9:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Keith Busch, qemu-block, Li Qiang
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();
--
2.19.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
2018-11-16 9:31 [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB Paolo Bonzini
@ 2018-11-16 10:38 ` Li Qiang
2018-11-16 13:10 ` no-reply
2018-11-19 15:23 ` Mark Kanda
2 siblings, 0 replies; 7+ messages in thread
From: Li Qiang @ 2018-11-16 10:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Qemu Developers, keith.busch, qemu-block
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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
2018-11-16 9:31 [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB Paolo Bonzini
2018-11-16 10:38 ` Li Qiang
@ 2018-11-16 13:10 ` no-reply
2018-11-19 15:23 ` Mark Kanda
2 siblings, 0 replies; 7+ messages in thread
From: no-reply @ 2018-11-16 13:10 UTC (permalink / raw)
To: pbonzini; +Cc: famz, qemu-devel, keith.busch, liq3ea, qemu-block
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
2018-11-16 9:31 [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB Paolo Bonzini
2018-11-16 10:38 ` Li Qiang
2018-11-16 13:10 ` no-reply
@ 2018-11-19 15:23 ` Mark Kanda
2018-11-19 17:09 ` Paolo Bonzini
2 siblings, 1 reply; 7+ messages in thread
From: Mark Kanda @ 2018-11-19 15:23 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Keith Busch, Li Qiang, qemu-block, kwolf
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();
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
2018-11-19 15:23 ` Mark Kanda
@ 2018-11-19 17:09 ` Paolo Bonzini
2018-11-19 17:43 ` Kevin Wolf
0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2018-11-19 17:09 UTC (permalink / raw)
To: Mark Kanda, qemu-devel; +Cc: Keith Busch, Li Qiang, qemu-block, kwolf
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();
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
2018-11-19 17:09 ` Paolo Bonzini
@ 2018-11-19 17:43 ` Kevin Wolf
2018-11-20 19:00 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2018-11-19 17:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Mark Kanda, qemu-devel, Keith Busch, Li Qiang, qemu-block
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
2018-11-19 17:43 ` Kevin Wolf
@ 2018-11-20 19:00 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2018-11-20 19:00 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Mark Kanda, qemu-devel, Keith Busch, Li Qiang, qemu-block
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-20 19:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-16 9:31 [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB Paolo Bonzini
2018-11-16 10:38 ` Li Qiang
2018-11-16 13:10 ` no-reply
2018-11-19 15:23 ` Mark Kanda
2018-11-19 17:09 ` Paolo Bonzini
2018-11-19 17:43 ` Kevin Wolf
2018-11-20 19:00 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.