Re: [PATCH] hw/nvme: fix cross-namespace copy dif buffer overflow

Klaus Jensen <[email protected]> Tue, 28 Jul 2026 11:47:36 +0200

Newsgroups	gmane.comp.emulators.qemu.block,gmane.comp.emulators.qemu,gmane.comp.emulators.qemu.stable
Message-ID	<[email protected]>

On Jul 28 11:32, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 28/7/26 11:18, Klaus Jensen wrote:
> > From: Klaus Jensen <[email protected]>
> > 
> > The NVMe specification allows a controller with multiple namespaces to
> > use different LBA formats per namespace. On implication of this is that
> 
> Typo "One".
> 
> > the destination namespace may have a metadata area for PI, but the
> > source does not. In that case, the controller shall generate the
> > protection information, but the bounce buffer is errornously allocated
> 
> Typo "erroneously".
> 

Thanks!

> > without space for that, causing a buffer overflow.
> > 
> > Fix the allocation.
> > 
> > Cc: [email protected]
> > Fixes: d522aef88d42 ("hw/nvme: add cross namespace copy support")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3387
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3692
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3841
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3853
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4031
> > Reported-by: Jihe Wang <[email protected]>
> > Reported-by: boy juju <[email protected]>
> > Reported-by: contact <[email protected]>
> > Reported-by: david korczynski <[email protected]>
> > Signed-off-by: Klaus Jensen <[email protected]>
> > ---
> >   hw/nvme/ctrl.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index a67e1598891c..409aeb9921c2 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -3332,7 +3332,8 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb)
> >       g_free(iocb->bounce);
> >       iocb->bounce = g_malloc_n(le16_to_cpu(sns->id_ns.mssrl),
> > -                              sns->lbasz + sns->lbaf.ms);
> > +                              sns->lbasz + (sns->lbaf.ms == 0 ? dns->lbaf.ms :
> > +                                            sns->lbaf.ms));
> 
>   sns->lbasz + (sns->lbaf.ms ? : dns->lbaf.ms)
> 
> Could MAX() be simpler?
> 

That was my first fix. But the case we are covering is actually only
when the source namespace metadata size is zero. Otherwise, the formats
must match (which is checked earlier). But since you are pointing it
out, I think MAX is probably more clear. We won't accidentially
over-allocate because the formats are checked earlier.

> >       qemu_iovec_reset(&iocb->iov);
> >       qemu_iovec_add(&iocb->iov, iocb->bounce, len);
> 
> Should we assert @len <= what we just allocated?
> 

Sure, would be reasonable!

> > 
> > ---
> > base-commit: 299e7557ed15a9a325620698add379a3ce2d1d95
> > change-id: 20260728-fix-cross-ns-pract-6934cc8205f2
> > 
> > Best regards,
> 
>

signature.asc (application/pgp-signature, 488 B)