[Qemu-devel] [PATCH] nvme: fix out-of-bounds access to the CMB

[Qemu-devel] [PATCH] nvme: fix out-of-bounds access to the CMB

[Paolo Bonzini]

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
Reported-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
Tested-by: 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      | 68 +++++++++++++++++++++++++++++++++++-------
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d0226e7fdc..ef046bbc54 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1199,7 +1199,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..2700ba838a 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -8,25 +8,73 @@
  */
 
 #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);
 }
 
-int main(int argc, char **argv)
+static void nvmetest_cmb_test(void)
 {
-    int ret;
+    const int cmb_bar_size = 2 * MiB;
+    QOSState *qs;
+    QPCIDevice *pdev;
+    QPCIBar bar;
 
-    g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/nvme/nop", nop);
+    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);
+    g_free(pdev);
 
-    qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
-                "-device nvme,drive=drv0,serial=foo");
-    ret = g_test_run();
+    qnvme_stop(qs);
+}
 
-    qtest_end();
+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);
 
-    return ret;
+    return g_test_run();
 }
-- 
2.19.1

Re: [Qemu-devel] [PATCH] nvme: fix out-of-bounds access to the CMB

[Philippe Mathieu-Daudé]

On 20/11/18 19:41, 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
> Reported-by: Li Qiang <liq3ea@gmail.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> Tested-by: 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      | 68 +++++++++++++++++++++++++++++++++++-------
>   3 files changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d0226e7fdc..ef046bbc54 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1199,7 +1199,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..2700ba838a 100644
> --- a/tests/nvme-test.c
> +++ b/tests/nvme-test.c
> @@ -8,25 +8,73 @@
>    */
>   
>   #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);
>   }
>   
> -int main(int argc, char **argv)
> +static void nvmetest_cmb_test(void)
>   {
> -    int ret;
> +    const int cmb_bar_size = 2 * MiB;
> +    QOSState *qs;
> +    QPCIDevice *pdev;
> +    QPCIBar bar;
>   
> -    g_test_init(&argc, &argv, NULL);
> -    qtest_add_func("/nvme/nop", nop);
> +    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);
> +    g_free(pdev);
>   
> -    qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
> -                "-device nvme,drive=drv0,serial=foo");
> -    ret = g_test_run();
> +    qnvme_stop(qs);
> +}
>   
> -    qtest_end();
> +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);
>   
> -    return ret;
> +    return g_test_run();
>   }
> 

TEST: tests/nvme-test... (pid=29324)
   /x86_64/nvme/nop:          OK
   /x86_64/nvme/cmb_test:     **
ERROR:tests/nvme-test.c:65:nvmetest_cmb_test: assertion failed 
(qpci_io_readb(pdev, bar, cmb_bar_size - 1) == 0x11): (0 == 17)
FAIL

Nice!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH] nvme: fix out-of-bounds access to the CMB

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20181120184148.22501-1-pbonzini@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH] 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
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1542868372-2602-1-git-send-email-liq3ea@gmail.com -> patchew/1542868372-2602-1-git-send-email-liq3ea@gmail.com
Switched to a new branch 'test'
506efa4 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)
#106: FILE: tests/nvme-test.c:53:
+    pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0));
                                                     ^

total: 1 errors, 0 warnings, 99 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

Re: [Qemu-devel] [PATCH] nvme: fix out-of-bounds access to the CMB

[Kevin Wolf]

Am 20.11.2018 um 19:41 hat Paolo Bonzini geschrieben:
> 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
> Reported-by: Li Qiang <liq3ea@gmail.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> Tested-by: Li Qiang <liq3ea@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks, applied to the block branch and reverted 5e3c0220d7.

Kevin

Re: [Qemu-devel] [PATCH] nvme: fix out-of-bounds access to the CMB

[Peter Maydell]

On 20 November 2018 at 18:41, Paolo Bonzini <pbonzini@redhat.com> 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.

Maybe we should change the MemoryRegionOps API so that
devices have to explicitly opt in to handling accesses
that span off the end of the region size they've registered?
IIRC we have one or two oddball devices that care about that
(probably mostly x86 IO port devices), but most device
implementations will not be expecting it.

I'm also surprised that the memory subsystem permits a
2 byte access at address sz-1 here, since .impl.unaligned
is not set...

thanks
-- PMM