| Message ID | 20240820044505.84005-4-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,for-9.1,1/1] hw/nvme: fix leak of uninitialized memory in io_mgmt_recv | expand |
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.
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.
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);