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

[Klaus Jensen]

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.

signature.asc
Description: PGP signature