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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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>