[PATCH-for-6.2 v2 0/2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)

[PATCH-for-6.2 v2 0/2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)

[Philippe Mathieu-Daudé]

Since v1:
- Do not add more buffer overflows in modify nvme_smart_info(),
  nvme_fw_log_info() and nvme_cmd_effects() (Klaus)
- Split nvme_error_info() change in another patch

Philippe Mathieu-Daudé (2):
  hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist
    (CVE-2021-3947)
  hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()

 hw/nvme/ctrl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

[Philippe Mathieu-Daudé]

Both 'buf_len' and 'off' arguments are under guest control.
Since nvme_c2h() doesn't check out of boundary access, the
caller must check for eventual buffer overrun on 'trans_len'.

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

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cfa..93a24464647 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4168,8 +4168,11 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     int i = 0;
     uint32_t nsid;
 
-    memset(nslist, 0x0, sizeof(nslist));
     trans_len = MIN(sizeof(nslist) - off, buf_len);
+    if (trans_len >= sizeof(nslist)) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+    memset(nslist, 0x0, sizeof(nslist));
 
     while ((nsid = find_first_bit(n->changed_nsids, NVME_CHANGED_NSID_SIZE)) !=
             NVME_CHANGED_NSID_SIZE) {
-- 
2.31.1

Re: [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

On Nov 17 13:35, Philippe Mathieu-Daudé wrote:
> Both 'buf_len' and 'off' arguments are under guest control.
> Since nvme_c2h() doesn't check out of boundary access, the
> caller must check for eventual buffer overrun on 'trans_len'.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Qiuhao Li <Qiuhao.Li@outlook.com>
> Fixes: f432fdfa121 ("support changed namespace asynchronous event")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvme/ctrl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 6a571d18cfa..93a24464647 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4168,8 +4168,11 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>      int i = 0;
>      uint32_t nsid;
>  
> -    memset(nslist, 0x0, sizeof(nslist));
>      trans_len = MIN(sizeof(nslist) - off, buf_len);
> +    if (trans_len >= sizeof(nslist)) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
> +    memset(nslist, 0x0, sizeof(nslist));
>  
>      while ((nsid = find_first_bit(n->changed_nsids, NVME_CHANGED_NSID_SIZE)) !=
>              NVME_CHANGED_NSID_SIZE) {

The issue I mentioned with off > sizeof(nslist). I'll send a fix that
just deals with it like all the other log pages.

There is probably a better way to do these checks, but I'm not sure how
right now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()

[Philippe Mathieu-Daudé]

Both 'buf_len' and 'off' arguments are under guest control.
Since nvme_c2h() doesn't check out of boundary access, the
caller must check for eventual buffer overrun on 'trans_len'.

Cc: qemu-stable@nongnu.org
Fixes: 94a7897c41d ("add support for the get log page command")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvme/ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 93a24464647..7414f3b4dd1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4146,7 +4146,8 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     uint32_t trans_len;
     NvmeErrorLog errlog;
 
-    if (off >= sizeof(errlog)) {
+    trans_len = MIN(sizeof(errlog) - off, buf_len);
+    if (trans_len >= sizeof(errlog)) {
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
@@ -4155,7 +4156,6 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     }
 
     memset(&errlog, 0x0, sizeof(errlog));
-    trans_len = MIN(sizeof(errlog) - off, buf_len);
 
     return nvme_c2h(n, (uint8_t *)&errlog, trans_len, req);
 }
-- 
2.31.1

Re: [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Nov 17 13:35, Philippe Mathieu-Daudé wrote:
> Both 'buf_len' and 'off' arguments are under guest control.
> Since nvme_c2h() doesn't check out of boundary access, the
> caller must check for eventual buffer overrun on 'trans_len'.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 94a7897c41d ("add support for the get log page command")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvme/ctrl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 93a24464647..7414f3b4dd1 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4146,7 +4146,8 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>      uint32_t trans_len;
>      NvmeErrorLog errlog;
>  
> -    if (off >= sizeof(errlog)) {
> +    trans_len = MIN(sizeof(errlog) - off, buf_len);
> +    if (trans_len >= sizeof(errlog)) {
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
>  
> @@ -4155,7 +4156,6 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>      }
>  
>      memset(&errlog, 0x0, sizeof(errlog));
> -    trans_len = MIN(sizeof(errlog) - off, buf_len);
>  
>      return nvme_c2h(n, (uint8_t *)&errlog, trans_len, req);
>  }
> -- 
> 2.31.1
> 

I don't see any buffer overrun issue prior to this patch. However, there
is a functional bug since the offset is not added in the c2h.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]