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

[Klaus Jensen <its@irrelevant.dk>]

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

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 ;)

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