1. Patchew
  2. QEMU
  3. View series

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

Philippe Mathieu-Daudé posted 1 patch 4 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211111153125.2258176-1-philmd@redhat.com
There is a newer version of this series
hw/nvme/ctrl.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
[PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
Posted by Philippe Mathieu-Daudé 4 years, 2 months ago 
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 | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cfa..634b290e069 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4072,7 +4072,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     NvmeNamespace *ns;
     time_t current_ms;
 
-    if (off >= sizeof(smart)) {
+    trans_len = MIN(sizeof(smart) - off, buf_len);
+    if (trans_len >= sizeof(smart)) {
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
@@ -4094,7 +4095,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
         }
     }
 
-    trans_len = MIN(sizeof(smart) - off, buf_len);
     smart.critical_warning = n->smart_critical_warning;
 
     smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
@@ -4130,12 +4130,12 @@ static uint16_t nvme_fw_log_info(NvmeCtrl *n, uint32_t buf_len, uint64_t off,
         .afi = 0x1,
     };
 
-    if (off >= sizeof(fw_log)) {
+    trans_len = MIN(sizeof(fw_log) - off, buf_len);
+    if (trans_len >= sizeof(fw_log)) {
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
     strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
-    trans_len = MIN(sizeof(fw_log) - off, buf_len);
 
     return nvme_c2h(n, (uint8_t *) &fw_log + off, trans_len, req);
 }
@@ -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(trans_len)) {
         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);
 }
@@ -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) {
@@ -4209,7 +4212,8 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
     const uint32_t *src_iocs = NULL;
     uint32_t trans_len;
 
-    if (off >= sizeof(log)) {
+    trans_len = MIN(sizeof(log) - off, buf_len);
+    if (trans_len >= sizeof(log)) {
         trace_pci_nvme_err_invalid_log_page_offset(off, sizeof(log));
         return NVME_INVALID_FIELD | NVME_DNR;
     }
@@ -4237,8 +4241,6 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
         memcpy(log.iocs, src_iocs, sizeof(log.iocs));
     }
 
-    trans_len = MIN(sizeof(log) - off, buf_len);
-
     return nvme_c2h(n, ((uint8_t *)&log) + off, trans_len, req);
 }
 
-- 
2.31.1
Re: [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
Posted by Klaus Jensen 4 years, 2 months ago 
On Nov 11 16:31, 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 | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 6a571d18cfa..634b290e069 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4072,7 +4072,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>      NvmeNamespace *ns;
>      time_t current_ms;
>  
> -    if (off >= sizeof(smart)) {
> +    trans_len = MIN(sizeof(smart) - off, buf_len);
> +    if (trans_len >= sizeof(smart)) {
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
>  
> @@ -4094,7 +4095,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>          }
>      }
>  
> -    trans_len = MIN(sizeof(smart) - off, buf_len);
>      smart.critical_warning = n->smart_critical_warning;
>  
>      smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,

Uhm. Hehe.

This "fix" breaks all log pages. Take smart_info as an example. Say the
offset is zero and the buffer length is 512. The transfer length
(trans_len) then becomes 512 and it ends up returning Invalid Field in
Command because trans_len equals sizeof(smart).

Worse, this "fix" actually *introduce* oob's all over the place if

    off > sizeof(smart) && buf_len < sizeof(smart)


Example

   sizeof(smart) = 512
   off = 516 (must be dword aligned to get to this spot)
   buf_len = 4 (same, but is always aligned)
   => trans_len = min(512 - 516, 4) = 4
   if (1 >= 512) => false

And we end up with

   nvme_c2h(n, &smart + 516, 4, req);



I suggest that we only fix nvme_changed_nslist ;)
Re: [PATCH-for-6.2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)
Posted by Philippe Mathieu-Daudé 4 years, 2 months ago 
On 11/11/21 19:08, Klaus Jensen wrote:
> On Nov 11 16:31, 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 | 22 ++++++++++++----------
>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> index 6a571d18cfa..634b290e069 100644
>> --- a/hw/nvme/ctrl.c
>> +++ b/hw/nvme/ctrl.c
>> @@ -4072,7 +4072,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>>      NvmeNamespace *ns;
>>      time_t current_ms;
>>  
>> -    if (off >= sizeof(smart)) {
>> +    trans_len = MIN(sizeof(smart) - off, buf_len);
>> +    if (trans_len >= sizeof(smart)) {
>>          return NVME_INVALID_FIELD | NVME_DNR;
>>      }
>>  
>> @@ -4094,7 +4095,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>>          }
>>      }
>>  
>> -    trans_len = MIN(sizeof(smart) - off, buf_len);
>>      smart.critical_warning = n->smart_critical_warning;
>>  
>>      smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
> 
> Uhm. Hehe.
> 
> This "fix" breaks all log pages. Take smart_info as an example. Say the
> offset is zero and the buffer length is 512. The transfer length
> (trans_len) then becomes 512 and it ends up returning Invalid Field in
> Command because trans_len equals sizeof(smart).
> 
> Worse, this "fix" actually *introduce* oob's all over the place if
> 
>     off > sizeof(smart) && buf_len < sizeof(smart)
> 
> 
> Example
> 
>    sizeof(smart) = 512
>    off = 516 (must be dword aligned to get to this spot)
>    buf_len = 4 (same, but is always aligned)
>    => trans_len = min(512 - 516, 4) = 4
>    if (1 >= 512) => false
> 
> And we end up with
> 
>    nvme_c2h(n, &smart + 516, 4, req);

Doh, I missed the '+ off' part in the nvme_c2h() call...

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.