Re: [PATCH 5.10 5.15] vfio/iommu_type1: replace kfree with kvfree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak in vfio_dma_bitmap_alloc_all error path leading to DoS/OOM
- [High] Out-of-bounds user memory access on 32-bit platforms due to incorrect pointer arithmetic in update_user_bitmap
--

commit 40ca6fabb082675b1379a40ebe33b025f5ba2193
Author: Jiacheng Shi <billsjc@sjtu.edu.cn>

vfio/iommu_type1: replace kfree with kvfree

Variables allocated by kvzalloc should not be freed by kfree, because they
may be allocated by vmalloc. This replaces kfree with kvfree for dma->bitmap.

Link: https://lore.kernel.org/r/20211212091600.2560-1-billsjc@sjtu.edu.cn

> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 67e8276389951..65df5bbc71a8b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -220,7 +220,7 @@ static int vfio_dma_bitmap_alloc(struct vfio_dma *dma, size_t pgsize)
>  
>  static void vfio_dma_bitmap_free(struct vfio_dma *dma)
>  {
> -	kfree(dma->bitmap);
> +	kvfree(dma->bitmap);
>  	dma->bitmap = NULL;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at the error path in
vfio_dma_bitmap_alloc_all() which calls this function:

drivers/vfio/vfio_iommu_type1.c:vfio_dma_bitmap_alloc_all() {
        ...
        for (p = rb_prev(n); p; p = rb_prev(p)) {
                struct vfio_dma *dma = rb_entry(n,
                                struct vfio_dma, node);
                vfio_dma_bitmap_free(dma);
        }
        ...
}

Does this code leak the previously allocated dma->bitmap memory?

The loop iterates backward using p, but it extracts the vfio_dma struct
using the failed node n instead of the loop iterator p. This causes the
error path to repeatedly free the failed node while leaking the successful
allocations.

Unprivileged users might be able to trigger this failure repeatedly via the
ioctl, which could exhaust memory.

[Severity: High]
This is a pre-existing issue, but can this pointer arithmetic cause
out-of-bounds user memory accesses on 32-bit platforms?

drivers/vfio/vfio_iommu_type1.c:update_user_bitmap() {
        ...
        unsigned long copy_offset = bit_offset / BITS_PER_LONG;
        ...
        if (copy_to_user((void __user *)(bitmap + copy_offset), dma->bitmap,
                         DIRTY_BITMAP_BYTES(nbits + shift)))
        ...
}

If the bitmap parameter is typed as a u64 pointer, advancing it by
copy_offset on 32-bit architectures (where BITS_PER_LONG is 32) will move
the pointer by copy_offset * 8 bytes.

Since copy_offset is bit_offset / 32, this advances the pointer by
bit_offset / 4 bytes instead of the expected bit_offset / 8 bytes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619150206.1719815-1-kniv@yandex-team.ru?part=1