1. Patchew
  2. QEMU
  3. View series

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

Klaus Jensen posted 1 patch 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260728-fix-cross-ns-pract-v1-1-20f66766337f@samsung.com
Maintainers: Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>, Jesper Devantier <foss@defmacro.it>
There is a newer version of this series
hw/nvme/ctrl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] hw/nvme: fix cross-namespace copy dif buffer overflow
Posted by Klaus Jensen 2 months ago 
From: Klaus Jensen <k.jensen@samsung.com>

The NVMe specification allows a controller with multiple namespaces to
use different LBA formats per namespace. On implication of this is that
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
without space for that, causing a buffer overflow.

Fix the allocation.

Cc: qemu-stable@nongnu.org
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 <wangjihe.mail@gmail.com>
Reported-by: boy juju <agx1657748706@gmail.com>
Reported-by: contact <contact@xchglabs.com>
Reported-by: david korczynski <david@adalogics.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 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));
 
     qemu_iovec_reset(&iocb->iov);
     qemu_iovec_add(&iocb->iov, iocb->bounce, len);

---
base-commit: 299e7557ed15a9a325620698add379a3ce2d1d95
change-id: 20260728-fix-cross-ns-pract-6934cc8205f2

Best regards,
-- 
Klaus Jensen <k.jensen@samsung.com>
Re: [PATCH] hw/nvme: fix cross-namespace copy dif buffer overflow
Posted by Philippe Mathieu-Daudé 2 months ago 
Hi Klaus,

On 28/7/26 11:18, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> 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".

> without space for that, causing a buffer overflow.
> 
> Fix the allocation.
> 
> Cc: qemu-stable@nongnu.org
> 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 <wangjihe.mail@gmail.com>
> Reported-by: boy juju <agx1657748706@gmail.com>
> Reported-by: contact <contact@xchglabs.com>
> Reported-by: david korczynski <david@adalogics.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   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?

>   
>       qemu_iovec_reset(&iocb->iov);
>       qemu_iovec_add(&iocb->iov, iocb->bounce, len);

Should we assert @len <= what we just allocated?

> 
> ---
> base-commit: 299e7557ed15a9a325620698add379a3ce2d1d95
> change-id: 20260728-fix-cross-ns-pract-6934cc8205f2
> 
> Best regards,
Re: [PATCH] hw/nvme: fix cross-namespace copy dif buffer overflow
Posted by Klaus Jensen 2 months ago 
On Jul 28 11:32, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 28/7/26 11:18, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > 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: qemu-stable@nongnu.org
> > 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 <wangjihe.mail@gmail.com>
> > Reported-by: boy juju <agx1657748706@gmail.com>
> > Reported-by: contact <contact@xchglabs.com>
> > Reported-by: david korczynski <david@adalogics.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   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,
> 
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.