Linux Virtualization - Jul 2019 - [PATCH 0/2] Fix NULL pointer dereference with virtio-blk-pci and virtual IOMMU

When running a guest featuring a virtio-blk-pci protected with a virtual
IOMMU we hit a NULL pointer dereference.

This series removes the dma_max_mapping_size() call in
virtio_max_dma_size when the device does not have any dma_mask set.
A check is also added to early return in dma_addressing_limited()
if the dma_mask is NULL.

Eric Auger (2):
  dma-mapping: Protect dma_addressing_limited against NULL dma_mask
  virtio/virtio_ring: Fix the dma_max_mapping_size call

 drivers/virtio/virtio_ring.c | 2 +-
 include/linux/dma-mapping.h  | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.20.1

dma_addressing_limited() should not be called on a device with
a NULL dma_mask. If this occurs let's WARN_ON_ONCE and immediately
return. Existing call sites are updated separately.

Fixes: b866455423e0 ("dma-mapping: add a dma_addressing_limited
helper")
Signed-off-by: Eric Auger <eric.auger at redhat.com>

---

v1 -> v2:
- add a WARN_ON_ONCE()
- reword the commit message
---
 include/linux/dma-mapping.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e11b115dd0e4..ef0cf9537abc 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -689,8 +689,9 @@ static inline int dma_coerce_mask_and_coherent(struct device
*dev, u64 mask)
  */
 static inline bool dma_addressing_limited(struct device *dev)
 {
-	return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
-		dma_get_required_mask(dev);
+	return WARN_ON_ONCE(!dev->dma_mask) ? false :
+		min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
+			dma_get_required_mask(dev);
 }
 
 #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
-- 
2.20.1

Do not call dma_max_mapping_size for devices that have no DMA
mask set, otherwise we can hit a NULL pointer dereference.

This occurs when a virtio-blk-pci device is protected with
a virtual IOMMU.

Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
Signed-off-by: Eric Auger <eric.auger at redhat.com>
Suggested-by: Christoph Hellwig <hch at lst.de>
---
 drivers/virtio/virtio_ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c8be1c4f5b55..37c143971211 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
 {
 	size_t max_segment_size = SIZE_MAX;
 
-	if (vring_use_dma_api(vdev))
+	if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
 		max_segment_size = dma_max_mapping_size(&vdev->dev);
 
 	return max_segment_size;
-- 
2.20.1

>  static inline bool dma_addressing_limited(struct device *dev)
>  {
> -	return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> -		dma_get_required_mask(dev);
> +	return WARN_ON_ONCE(!dev->dma_mask) ? false :
> +		min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> +			dma_get_required_mask(dev);
This should really use a separate if statement, but I can fix that
up when applying it.

On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger
wrote:
> Do not call dma_max_mapping_size for devices that have no DMA
> mask set, otherwise we can hit a NULL pointer dereference.
> 
> This occurs when a virtio-blk-pci device is protected with
> a virtual IOMMU.
> 
> Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> Signed-off-by: Eric Auger <eric.auger at redhat.com>
> Suggested-by: Christoph Hellwig <hch at lst.de>
Looks good.  virtio maintainers, let me know if you want to queue
it up or if I should pick the patch up through the dma-mapping tree.

On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger
wrote:
> Do not call dma_max_mapping_size for devices that have no DMA
> mask set, otherwise we can hit a NULL pointer dereference.
> 
> This occurs when a virtio-blk-pci device is protected with
> a virtual IOMMU.
> 
> Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> Signed-off-by: Eric Auger <eric.auger at redhat.com>
> Suggested-by: Christoph Hellwig <hch at lst.de>
Christoph, I wonder why did you suggest this?
The connection between dma_mask and dma_max_mapping_size
is far from obvious.  The documentation doesn't exist.
Do we really have to teach all users about this hack?
Why not just make dma_max_mapping_size DTRT?
> ---
>  drivers/virtio/virtio_ring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index c8be1c4f5b55..37c143971211 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
>  {
>  	size_t max_segment_size = SIZE_MAX;
>  
> -	if (vring_use_dma_api(vdev))
> +	if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
>  		max_segment_size = dma_max_mapping_size(&vdev->dev);
>  
>  	return max_segment_size;
> -- 
> 2.20.1

On 22/07/2019 15:55, Eric Auger wrote:
> Do not call dma_max_mapping_size for devices that have no DMA
> mask set, otherwise we can hit a NULL pointer dereference.
> 
> This occurs when a virtio-blk-pci device is protected with
> a virtual IOMMU.
> 
> Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> Signed-off-by: Eric Auger <eric.auger at redhat.com>
> Suggested-by: Christoph Hellwig <hch at lst.de>
> ---
>   drivers/virtio/virtio_ring.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index c8be1c4f5b55..37c143971211 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
>   {
>   	size_t max_segment_size = SIZE_MAX;
>   
> -	if (vring_use_dma_api(vdev))
> +	if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
Hmm, might it make sense to roll that check up into vring_use_dma_api() 
itself? After all, if the device has no mask then it's likely that other 
DMA API ops wouldn't really work as expected either.

Robin.
>   		max_segment_size = dma_max_mapping_size(&vdev->dev);
>   
>   	return max_segment_size;
>