[PULL for-6.2 0/3] hw/nvme fixes

[PULL for-6.2 0/3] hw/nvme fixes

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Hi,

The following changes since commit 44a3aa0608f01274418487b655d42467c1d8334e:

  Merge tag 'sev-hashes-pull-request' of https://gitlab.com/berrange/qemu into staging (2021-11-18 15:06:05 +0100)

are available in the Git repository at:

  git://git.infradead.org/qemu-nvme.git tags/nvme-fixes-for-6.2-pull-request

for you to fetch changes up to e2c57529c9306e4c9aac75d9879f6e7699584a22:

  hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947) (2021-11-19 07:32:19 +0100)

----------------------------------------------------------------
hw/nvme fixes

* Fix CVE-2021-3947
* Controller hotplugging fixes

----------------------------------------------------------------

Hannes Reinecke (1):
  hw/nvme: reattach subsystem namespaces on hotplug

Klaus Jensen (2):
  hw/nvme: change nvme-ns 'shared' default
  hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

 docs/system/devices/nvme.rst | 24 ++++++++++++++----------
 hw/core/machine.c            |  1 +
 hw/nvme/ctrl.c               |  5 +++++
 hw/nvme/ns.c                 |  8 +-------
 hw/nvme/subsys.c             | 10 +++++++++-
 5 files changed, 30 insertions(+), 18 deletions(-)

-- 
2.34.0

[PULL for-6.2 1/3] hw/nvme: reattach subsystem namespaces on hotplug

[Klaus Jensen]

From: Hannes Reinecke <hare@suse.de>

With commit 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
namespaces get moved from the controller to the subsystem if one
is specified.
That keeps the namespaces alive after a controller hot-unplug, but
after a controller hotplug we have to reconnect the namespaces
from the subsystem to the controller.

Fixes: 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
Cc: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Hannes Reinecke <hare@suse.de>
[k.jensen: only attach to shared and non-detached namespaces]
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/subsys.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
index 495dcff5ebb8..fb58d639504e 100644
--- a/hw/nvme/subsys.c
+++ b/hw/nvme/subsys.c
@@ -14,7 +14,7 @@
 int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
 {
     NvmeSubsystem *subsys = n->subsys;
-    int cntlid;
+    int cntlid, nsid;
 
     for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) {
         if (!subsys->ctrls[cntlid]) {
@@ -29,12 +29,20 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
 
     subsys->ctrls[cntlid] = n;
 
+    for (nsid = 1; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) {
+        NvmeNamespace *ns = subsys->namespaces[nsid];
+        if (ns && ns->params.shared && !ns->params.detached) {
+            nvme_attach_ns(n, ns);
+        }
+    }
+
     return cntlid;
 }
 
 void nvme_subsys_unregister_ctrl(NvmeSubsystem *subsys, NvmeCtrl *n)
 {
     subsys->ctrls[n->cntlid] = NULL;
+    n->cntlid = -1;
 }
 
 static void nvme_subsys_setup(NvmeSubsystem *subsys)
-- 
2.34.0

[PULL for-6.2 2/3] hw/nvme: change nvme-ns 'shared' default

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Change namespaces to be shared namespaces by default (parameter
shared=on). Keep shared=off for older machine types.

Reviewed-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 docs/system/devices/nvme.rst | 24 ++++++++++++++----------
 hw/core/machine.c            |  1 +
 hw/nvme/ns.c                 |  8 +-------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
index bff72d1c24d0..a1c0db01f6d5 100644
--- a/docs/system/devices/nvme.rst
+++ b/docs/system/devices/nvme.rst
@@ -110,28 +110,32 @@ multipath I/O.
 This will create an NVM subsystem with two controllers. Having controllers
 linked to an ``nvme-subsys`` device allows additional ``nvme-ns`` parameters:
 
-``shared`` (default: ``off``)
+``shared`` (default: ``on`` since 6.2)
   Specifies that the namespace will be attached to all controllers in the
-  subsystem. If set to ``off`` (the default), the namespace will remain a
-  private namespace and may only be attached to a single controller at a time.
+  subsystem. If set to ``off``, the namespace will remain a private namespace
+  and may only be attached to a single controller at a time. Shared namespaces
+  are always automatically attached to all controllers (also when controllers
+  are hotplugged).
 
 ``detached`` (default: ``off``)
   If set to ``on``, the namespace will be be available in the subsystem, but
-  not attached to any controllers initially.
+  not attached to any controllers initially. A shared namespace with this set
+  to ``on`` will never be automatically attached to controllers.
 
 Thus, adding
 
 .. code-block:: console
 
    -drive file=nvm-1.img,if=none,id=nvm-1
-   -device nvme-ns,drive=nvm-1,nsid=1,shared=on
+   -device nvme-ns,drive=nvm-1,nsid=1
    -drive file=nvm-2.img,if=none,id=nvm-2
-   -device nvme-ns,drive=nvm-2,nsid=3,detached=on
+   -device nvme-ns,drive=nvm-2,nsid=3,shared=off,detached=on
 
-will cause NSID 1 will be a shared namespace (due to ``shared=on``) that is
-initially attached to both controllers. NSID 3 will be a private namespace
-(i.e. only attachable to a single controller at a time) and will not be
-attached to any controller initially (due to ``detached=on``).
+will cause NSID 1 will be a shared namespace that is initially attached to both
+controllers. NSID 3 will be a private namespace due to ``shared=off`` and only
+attachable to a single controller at a time. Additionally it will not be
+attached to any controller initially (due to ``detached=on``) or to hotplugged
+controllers.
 
 Optional Features
 =================
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 26ec54e7261c..53a99abc5605 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@
 
 GlobalProperty hw_compat_6_1[] = {
     { "vhost-user-vsock-device", "seqpacket", "off" },
+    { "nvme-ns", "shared", "off" },
 };
 const size_t hw_compat_6_1_len = G_N_ELEMENTS(hw_compat_6_1);
 
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index b7cf1494e75b..8b5f98c76180 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -465,12 +465,6 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
                        "linked to an nvme-subsys device");
             return;
         }
-
-        if (ns->params.shared) {
-            error_setg(errp, "shared requires that the nvme device is "
-                       "linked to an nvme-subsys device");
-            return;
-        }
     } else {
         /*
          * If this namespace belongs to a subsystem (through a link on the
@@ -532,7 +526,7 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
 static Property nvme_ns_props[] = {
     DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
     DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false),
-    DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
+    DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, true),
     DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
     DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
     DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0),
-- 
2.34.0

[PULL for-6.2 3/3] hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Fix missing offset verification.

Cc: qemu-stable@nongnu.org
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
Fixes: f432fdfa121 ("support changed namespace asynchronous event")
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cfae..5f573c417b3d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4168,6 +4168,11 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     int i = 0;
     uint32_t nsid;
 
+    if (off >= sizeof(nslist)) {
+        trace_pci_nvme_err_invalid_log_page_offset(off, sizeof(nslist));
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+
     memset(nslist, 0x0, sizeof(nslist));
     trans_len = MIN(sizeof(nslist) - off, buf_len);
 
-- 
2.34.0

Re: [PULL for-6.2 0/3] hw/nvme fixes

[Richard Henderson]

On 11/19/21 9:09 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Hi,
> 
> The following changes since commit 44a3aa0608f01274418487b655d42467c1d8334e:
> 
>    Merge tag 'sev-hashes-pull-request' of https://gitlab.com/berrange/qemu into staging (2021-11-18 15:06:05 +0100)
> 
> are available in the Git repository at:
> 
>    git://git.infradead.org/qemu-nvme.git tags/nvme-fixes-for-6.2-pull-request
> 
> for you to fetch changes up to e2c57529c9306e4c9aac75d9879f6e7699584a22:
> 
>    hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947) (2021-11-19 07:32:19 +0100)
> 
> ----------------------------------------------------------------
> hw/nvme fixes
> 
> * Fix CVE-2021-3947
> * Controller hotplugging fixes
> 
> ----------------------------------------------------------------
> 
> Hannes Reinecke (1):
>    hw/nvme: reattach subsystem namespaces on hotplug
> 
> Klaus Jensen (2):
>    hw/nvme: change nvme-ns 'shared' default
>    hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)
> 
>   docs/system/devices/nvme.rst | 24 ++++++++++++++----------
>   hw/core/machine.c            |  1 +
>   hw/nvme/ctrl.c               |  5 +++++
>   hw/nvme/ns.c                 |  8 +-------
>   hw/nvme/subsys.c             | 10 +++++++++-
>   5 files changed, 30 insertions(+), 18 deletions(-)

Applied, thanks.

r~