hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

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

Posted by Klaus Jensen 2 years, 5 months ago

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")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---

Note: Since its so easy to mess this fix up, the log pages code could
probably use a refactor - there is a lot of duplicated code as well and
it's easy to miss a check like this. However, defer that for 7.0.

 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: [PATCH for-6.2] hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

Posted by Philippe Mathieu-Daudé 2 years, 5 months ago

On 11/17/21 14:23, Klaus Jensen wrote:
> 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")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

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

> ---
> 
> Note: Since its so easy to mess this fix up, the log pages code could
> probably use a refactor - there is a lot of duplicated code as well and
> it's easy to miss a check like this. However, defer that for 7.0.
> 
>  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);
>  
>