[PATCH] block/nvme: Fix VFIO_MAP_DMA failed: No space left on device

[PATCH] block/nvme: Fix VFIO_MAP_DMA failed: No space left on device

[Philippe Mathieu-Daudé]

When the NVMe block driver was introduced (see commit bdd6a90a9e5,
January 2018), Linux VFIO_IOMMU_MAP_DMA ioctl was only returning
-ENOMEM in case of error. The driver was correctly handling the
error path to recycle its volatile IOVA mappings.

To fix CVE-2019-3882, Linux commit 492855939bdb ("vfio/type1: Limit
DMA mappings per container", April 2019) added the -ENOSPC error to
signal the user exhausted the DMA mappings available for a container.

The block driver started to mis-behave:

  qemu-system-x86_64: VFIO_MAP_DMA failed: No space left on device
  (qemu)
  (qemu) info status
  VM status: paused (io-error)
  (qemu) c
  VFIO_MAP_DMA failed: No space left on device
  qemu-system-x86_64: block/block-backend.c:1968: blk_get_aio_context: Assertion `ctx == blk->ctx' failed.

Fix by handling the -ENOSPC error when DMA mappings are exhausted;
other errors (such -ENOMEM) are still handled later in the same
function.

An easy way to reproduce this bug is to restrict the DMA mapping
limit (65535 by default) when loading the VFIO IOMMU module:

  # modprobe vfio_iommu_type1 dma_entry_limit=666

Cc: qemu-stable@nongnu.org
Reported-by: Michal Prívozník <mprivozn@redhat.com>
Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
Buglink: https://bugs.launchpad.net/qemu/+bug/1863333
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/65
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Michal, is it still possible for you to test this (old bug)?

A functional test using viommu & nested VM is planned (suggested by
Stefan and Maxim).
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 2b5421e7aa6..12f9dd5cce3 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1030,7 +1030,7 @@ try_map:
         r = qemu_vfio_dma_map(s->vfio,
                               qiov->iov[i].iov_base,
                               len, true, &iova);
-        if (r == -ENOMEM && retry) {
+        if (r == -ENOSPC && retry) {
             retry = false;
             trace_nvme_dma_flush_queue_wait(s);
             if (s->dma_map_count) {
-- 
2.31.1

Re: [PATCH] block/nvme: Fix VFIO_MAP_DMA failed: No space left on device

[Michal Prívozník]

On 6/11/21 1:46 PM, Philippe Mathieu-Daudé wrote:
> When the NVMe block driver was introduced (see commit bdd6a90a9e5,
> January 2018), Linux VFIO_IOMMU_MAP_DMA ioctl was only returning
> -ENOMEM in case of error. The driver was correctly handling the
> error path to recycle its volatile IOVA mappings.
> 
> To fix CVE-2019-3882, Linux commit 492855939bdb ("vfio/type1: Limit
> DMA mappings per container", April 2019) added the -ENOSPC error to
> signal the user exhausted the DMA mappings available for a container.
> 
> The block driver started to mis-behave:
> 
>   qemu-system-x86_64: VFIO_MAP_DMA failed: No space left on device
>   (qemu)
>   (qemu) info status
>   VM status: paused (io-error)
>   (qemu) c
>   VFIO_MAP_DMA failed: No space left on device
>   qemu-system-x86_64: block/block-backend.c:1968: blk_get_aio_context: Assertion `ctx == blk->ctx' failed.
> 
> Fix by handling the -ENOSPC error when DMA mappings are exhausted;
> other errors (such -ENOMEM) are still handled later in the same
> function.
> 
> An easy way to reproduce this bug is to restrict the DMA mapping
> limit (65535 by default) when loading the VFIO IOMMU module:
> 
>   # modprobe vfio_iommu_type1 dma_entry_limit=666
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Michal Prívozník <mprivozn@redhat.com>
> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
> Buglink: https://bugs.launchpad.net/qemu/+bug/1863333
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/65
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Michal, is it still possible for you to test this (old bug)?
> 

Unfortunately I no longer have access to the machine. But, IIRC it was
fairly easy to reproduce - just passthrough any NVMe disk using NVMe
disk backend (-blockdev '{"driver":"nvme", ...).

Sorry,
Michal

Re: [PATCH] block/nvme: Fix VFIO_MAP_DMA failed: No space left on device

[Philippe Mathieu-Daudé]

On 6/11/21 1:46 PM, Philippe Mathieu-Daudé wrote:
> When the NVMe block driver was introduced (see commit bdd6a90a9e5,
> January 2018), Linux VFIO_IOMMU_MAP_DMA ioctl was only returning
> -ENOMEM in case of error. The driver was correctly handling the
> error path to recycle its volatile IOVA mappings.
> 
> To fix CVE-2019-3882, Linux commit 492855939bdb ("vfio/type1: Limit
> DMA mappings per container", April 2019) added the -ENOSPC error to
> signal the user exhausted the DMA mappings available for a container.

Hmm this commit has been added before v5.1-rc4.

So while this fixes the behavior of v5.1-rc4+ kernels,
older kernels using this fix will have the same problem...

Should I check uname(2)'s utsname.release[]? Is it reliable?

> The block driver started to mis-behave:
> 
>   qemu-system-x86_64: VFIO_MAP_DMA failed: No space left on device
>   (qemu)
>   (qemu) info status
>   VM status: paused (io-error)
>   (qemu) c
>   VFIO_MAP_DMA failed: No space left on device
>   qemu-system-x86_64: block/block-backend.c:1968: blk_get_aio_context: Assertion `ctx == blk->ctx' failed.
> 
> Fix by handling the -ENOSPC error when DMA mappings are exhausted;
> other errors (such -ENOMEM) are still handled later in the same
> function.
> 
> An easy way to reproduce this bug is to restrict the DMA mapping
> limit (65535 by default) when loading the VFIO IOMMU module:
> 
>   # modprobe vfio_iommu_type1 dma_entry_limit=666
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Michal Prívozník <mprivozn@redhat.com>
> Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
> Buglink: https://bugs.launchpad.net/qemu/+bug/1863333
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/65
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Michal, is it still possible for you to test this (old bug)?
> 
> A functional test using viommu & nested VM is planned (suggested by
> Stefan and Maxim).
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 2b5421e7aa6..12f9dd5cce3 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -1030,7 +1030,7 @@ try_map:
>          r = qemu_vfio_dma_map(s->vfio,
>                                qiov->iov[i].iov_base,
>                                len, true, &iova);
> -        if (r == -ENOMEM && retry) {
> +        if (r == -ENOSPC && retry) {
>              retry = false;
>              trace_nvme_dma_flush_queue_wait(s);
>              if (s->dma_map_count) {
> 

Re: [PATCH] block/nvme: Fix VFIO_MAP_DMA failed: No space left on device

[Michal Prívozník]

On 6/14/21 6:03 PM, Philippe Mathieu-Daudé wrote:
> On 6/11/21 1:46 PM, Philippe Mathieu-Daudé wrote:
>> When the NVMe block driver was introduced (see commit bdd6a90a9e5,
>> January 2018), Linux VFIO_IOMMU_MAP_DMA ioctl was only returning
>> -ENOMEM in case of error. The driver was correctly handling the
>> error path to recycle its volatile IOVA mappings.
>>
>> To fix CVE-2019-3882, Linux commit 492855939bdb ("vfio/type1: Limit
>> DMA mappings per container", April 2019) added the -ENOSPC error to
>> signal the user exhausted the DMA mappings available for a container.
> 
> Hmm this commit has been added before v5.1-rc4.
> 
> So while this fixes the behavior of v5.1-rc4+ kernels,
> older kernels using this fix will have the same problem...
> 
> Should I check uname(2)'s utsname.release[]? Is it reliable?

Not at all. What if somebody runs kernel with that commit backported? I
would leave this up to distro maintainers to figure out. They know what
kernel patches are backported and thus what qemu patches should be
backported too. I'm wondering if there's a way we can help them by
letting know (other than mentioning the kernel commit).

Michal

Re: [PATCH] block/nvme: Fix VFIO_MAP_DMA failed: No space left on device

[Maxim Levitsky]

On Mon, 2021-06-14 at 18:03 +0200, Philippe Mathieu-Daudé wrote:
> On 6/11/21 1:46 PM, Philippe Mathieu-Daudé wrote:
> > When the NVMe block driver was introduced (see commit bdd6a90a9e5,
> > January 2018), Linux VFIO_IOMMU_MAP_DMA ioctl was only returning
> > -ENOMEM in case of error. The driver was correctly handling the
> > error path to recycle its volatile IOVA mappings.
> > 
> > To fix CVE-2019-3882, Linux commit 492855939bdb ("vfio/type1: Limit
> > DMA mappings per container", April 2019) added the -ENOSPC error to
> > signal the user exhausted the DMA mappings available for a container.
> 
> Hmm this commit has been added before v5.1-rc4.
> 
> So while this fixes the behavior of v5.1-rc4+ kernels,
> older kernels using this fix will have the same problem...


Hi!

I wonder why not to check for both -ENOMEM and -ENOSPC
and recycle the mappings in both cases?

I think that would work on both old and new kernels.

What do you think?

Best regards,
	Maxim Levitsky

> 
> Should I check uname(2)'s utsname.release[]? Is it reliable?
> 
> > The block driver started to mis-behave:
> > 
> >   qemu-system-x86_64: VFIO_MAP_DMA failed: No space left on device
> >   (qemu)
> >   (qemu) info status
> >   VM status: paused (io-error)
> >   (qemu) c
> >   VFIO_MAP_DMA failed: No space left on device
> >   qemu-system-x86_64: block/block-backend.c:1968: blk_get_aio_context: Assertion `ctx == blk->ctx' failed.
> > 
> > Fix by handling the -ENOSPC error when DMA mappings are exhausted;
> > other errors (such -ENOMEM) are still handled later in the same
> > function.
> > 
> > An easy way to reproduce this bug is to restrict the DMA mapping
> > limit (65535 by default) when loading the VFIO IOMMU module:
> > 
> >   # modprobe vfio_iommu_type1 dma_entry_limit=666
> > 
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Michal Prívozník <mprivozn@redhat.com>
> > Fixes: bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1863333
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/65
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > Michal, is it still possible for you to test this (old bug)?
> > 
> > A functional test using viommu & nested VM is planned (suggested by
> > Stefan and Maxim).
> > ---
> >  block/nvme.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/block/nvme.c b/block/nvme.c
> > index 2b5421e7aa6..12f9dd5cce3 100644
> > --- a/block/nvme.c
> > +++ b/block/nvme.c
> > @@ -1030,7 +1030,7 @@ try_map:
> >          r = qemu_vfio_dma_map(s->vfio,
> >                                qiov->iov[i].iov_base,
> >                                len, true, &iova);
> > -        if (r == -ENOMEM && retry) {
> > +        if (r == -ENOSPC && retry) {
> >              retry = false;
> >              trace_nvme_dma_flush_queue_wait(s);
> >              if (s->dma_map_count) {
> > 

Re: [PATCH] block/nvme: Fix VFIO_MAP_DMA failed: No space left on device

[Philippe Mathieu-Daudé]

On 6/17/21 2:40 PM, Maxim Levitsky wrote:
> On Mon, 2021-06-14 at 18:03 +0200, Philippe Mathieu-Daudé wrote:
>> On 6/11/21 1:46 PM, Philippe Mathieu-Daudé wrote:
>>> When the NVMe block driver was introduced (see commit bdd6a90a9e5,
>>> January 2018), Linux VFIO_IOMMU_MAP_DMA ioctl was only returning
>>> -ENOMEM in case of error. The driver was correctly handling the
>>> error path to recycle its volatile IOVA mappings.
>>>
>>> To fix CVE-2019-3882, Linux commit 492855939bdb ("vfio/type1: Limit
>>> DMA mappings per container", April 2019) added the -ENOSPC error to
>>> signal the user exhausted the DMA mappings available for a container.
>>
>> Hmm this commit has been added before v5.1-rc4.
>>
>> So while this fixes the behavior of v5.1-rc4+ kernels,
>> older kernels using this fix will have the same problem...
> 
> 
> Hi!
> 
> I wonder why not to check for both -ENOMEM and -ENOSPC
> and recycle the mappings in both cases?
> 
> I think that would work on both old and new kernels.
> 
> What do you think?

Yes, worst case we retry one more time for nothing.

Alex suggested to use VFIO_IOMMU_GET_INFO to get the limit
with VFIO_IOMMU_TYPE1_INFO_DMA_AVAIL, and if not available
the driver should use a reasonable value to limit itself.

I'll try to get it quick, otherwise fall back to your "dual
errno" case.

Thanks both for the ideas,

Phil.

>>> diff --git a/block/nvme.c b/block/nvme.c
>>> index 2b5421e7aa6..12f9dd5cce3 100644
>>> --- a/block/nvme.c
>>> +++ b/block/nvme.c
>>> @@ -1030,7 +1030,7 @@ try_map:
>>>          r = qemu_vfio_dma_map(s->vfio,
>>>                                qiov->iov[i].iov_base,
>>>                                len, true, &iova);
>>> -        if (r == -ENOMEM && retry) {
>>> +        if (r == -ENOSPC && retry) {
>>>              retry = false;
>>>              trace_nvme_dma_flush_queue_wait(s);
>>>              if (s->dma_map_count) {
>>>