[PULL for-9.1 0/1] hw/nvme late fix

[PULL for-9.1 0/1] hw/nvme late fix

[Klaus Jensen]

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

Hi,

The following changes since commit 48e4ba59a3756aad743982da16bf9b5120d91a0c:

  Merge tag 'pull-riscv-to-apply-20240819-1' of https://github.com/alistair23/qemu into staging (2024-08-19 14:55:23 +1000)

are available in the Git repository at:

  https://gitlab.com/birkelund/qemu.git tags/nvme-next-pull-request

for you to fetch changes up to 6a22121c4f25b181e99479f65958ecde65da1c92:

  hw/nvme: fix leak of uninitialized memory in io_mgmt_recv (2024-08-20 06:16:48 +0200)

----------------------------------------------------------------
hw/nvme late fix
-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEUigzqnXi3OaiR2bATeGvMW1PDekFAmbEHsUACgkQTeGvMW1P
DenlQgf/dzz4B5pzdD0HsjNVNulxygAJEnYitiF/50LRj564hQDoisNYPvHeKMA7
wfk8jSSimTM6YkETksiR2DvnXlZ3wXn/HAhqE15GSW8vtRK2/RO9vNn51gyoFvl3
z/Wm8ahoFaNpygQQkQMIJ9QHVD3GheZH4OxMhqI1523+s7dGcUNetoZiyoBAdJ6m
7KOa/zUTPBmvpKMOEa25Ss+nZIPp9eFuCwQxhToV0gEuJFHolRZYv7GA4UjnodvJ
HrBrbsB8W4vh65FmC7WLAG9XFvNMgC0h8qtzWyKhNcxf478E7FckLvnAzSZExitj
fJzrSJV0bJHlQEM2q0yHYpL0urh5XA==
=ZeRF
-----END PGP SIGNATURE-----

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

Klaus Jensen (1):
  hw/nvme: fix leak of uninitialized memory in io_mgmt_recv

 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.45.2

[PULL for-9.1 1/1] hw/nvme: fix leak of uninitialized memory in io_mgmt_recv

[Klaus Jensen]

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

Yutaro Shimizu from the Cyber Defense Institute discovered a bug in the
NVMe emulation that leaks contents of an uninitialized heap buffer if
subsystem and FDP emulation are enabled.

Cc: qemu-stable@nongnu.org
Reported-by: Yutaro Shimizu <shimizu@cyberdefense.jp>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index c6d4f61a47f9..9f277b81d83c 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4474,7 +4474,7 @@ static uint16_t nvme_io_mgmt_recv_ruhs(NvmeCtrl *n, NvmeRequest *req,
 
     nruhsd = ns->fdp.nphs * endgrp->fdp.nrg;
     trans_len = sizeof(NvmeRuhStatus) + nruhsd * sizeof(NvmeRuhStatusDescr);
-    buf = g_malloc(trans_len);
+    buf = g_malloc0(trans_len);
 
     trans_len = MIN(trans_len, len);
 
-- 
2.45.2

Re: [PULL for-9.1 1/1] hw/nvme: fix leak of uninitialized memory in io_mgmt_recv

[Philippe Mathieu-Daudé]

Hi Klaus,

On 20/8/24 06:45, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Yutaro Shimizu from the Cyber Defense Institute discovered a bug in the
> NVMe emulation that leaks contents of an uninitialized heap buffer if
> subsystem and FDP emulation are enabled.

Was this patch posted on the list for review?

Usually we log here backtrace, reproducers.

Which fields are leaked?

Let's avoid the proven unsafe security by obscurity.

> Cc: qemu-stable@nongnu.org
> Reported-by: Yutaro Shimizu <shimizu@cyberdefense.jp>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index c6d4f61a47f9..9f277b81d83c 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4474,7 +4474,7 @@ static uint16_t nvme_io_mgmt_recv_ruhs(NvmeCtrl *n, NvmeRequest *req,
>   
>       nruhsd = ns->fdp.nphs * endgrp->fdp.nrg;
>       trans_len = sizeof(NvmeRuhStatus) + nruhsd * sizeof(NvmeRuhStatusDescr);
> -    buf = g_malloc(trans_len);
> +    buf = g_malloc0(trans_len);
>   
>       trans_len = MIN(trans_len, len);

The malloc could be done after finding the min length.

Shouldn't we check len is big enough?

Thanks,

Phil.

Re: [PULL for-9.1 1/1] hw/nvme: fix leak of uninitialized memory in io_mgmt_recv

[Klaus Jensen]

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

On Aug 20 12:30, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 20/8/24 06:45, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Yutaro Shimizu from the Cyber Defense Institute discovered a bug in the
> > NVMe emulation that leaks contents of an uninitialized heap buffer if
> > subsystem and FDP emulation are enabled.
> 
> Was this patch posted on the list for review?
> 

I wanted to get this into -rc3, so I might have jumped the gun. Didn't
add internal Reviewed-by (I should have done that). Jesper reviewed it.

Reviewed-by: Jesper Devantier <j.devantier@samsung.com>

> Usually we log here backtrace, reproducers.

It doesn't crash anything, so no backtrace; but Yutaro did provide a poc
to show the leak - those are on qemu-security and I did not request
permission to make that public.

I realize that my commit message is too brief; I will add that and post
as PATCH instead.

> 
> Which fields are leaked?

Parts of the NvmeRuHandle descriptor are reserved - they are not set
explicitly here, so they end up leaking heap content.

> 
> Let's avoid the proven unsafe security by obscurity.

Apologies - that was not my intention!

> 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Yutaro Shimizu <shimizu@cyberdefense.jp>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/nvme/ctrl.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index c6d4f61a47f9..9f277b81d83c 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -4474,7 +4474,7 @@ static uint16_t nvme_io_mgmt_recv_ruhs(NvmeCtrl *n, NvmeRequest *req,
> >       nruhsd = ns->fdp.nphs * endgrp->fdp.nrg;
> >       trans_len = sizeof(NvmeRuhStatus) + nruhsd * sizeof(NvmeRuhStatusDescr);
> > -    buf = g_malloc(trans_len);
> > +    buf = g_malloc0(trans_len);
> >       trans_len = MIN(trans_len, len);
> 
> The malloc could be done after finding the min length.
> 

Yes we could do that but it complicates building the data structure.
Here, what we do is build the data structure to be returned in full, and
then we transfer the minimum of what the host requested or the size of
that data structure.

Regardless, zeroing the memory somehow is required. We could also set
the reserved field to 0 explicitly, but g_malloc0 is more clear I think.

> Shouldn't we check len is big enough?

len is a host provided number of bytes. It does not have to be big
enough to fit the data structure; we transfer the minimum of the data
structure or what the host requested. 

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

Re: [PULL for-9.1 0/1] hw/nvme late fix

[Richard Henderson]

On 8/20/24 14:45, Klaus Jensen wrote:
> From: Klaus Jensen<k.jensen@samsung.com>
> 
> Hi,
> 
> The following changes since commit 48e4ba59a3756aad743982da16bf9b5120d91a0c:
> 
>    Merge tag 'pull-riscv-to-apply-20240819-1' ofhttps://github.com/alistair23/qemu into staging (2024-08-19 14:55:23 +1000)
> 
> are available in the Git repository at:
> 
>    https://gitlab.com/birkelund/qemu.git tags/nvme-next-pull-request
> 
> for you to fetch changes up to 6a22121c4f25b181e99479f65958ecde65da1c92:
> 
>    hw/nvme: fix leak of uninitialized memory in io_mgmt_recv (2024-08-20 06:16:48 +0200)

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/9.1 as appropriate.

r~