Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-16 Thread Christoph Hellwig 
On Tue, Sep 15, 2020 at 04:46:17PM -0400, Thomas Tai wrote:
> I tried out the suggested changes, and it successfully warned the null 
> pointer without panic. I notice that there are some places outside the 
> dma-direct, which calls dma_capable().
>
> https://elixir.bootlin.com/linux/v5.9-rc5/source/arch/x86/kernel/amd_gart_64.c#L187
>
> https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/xen/swiotlb-xen.c#L387

All of these still come in throught the wrappers in kernel/dma/mapping.c.

> Given that the WARN_ON_ONCE already did the intended warning, would you be 
> ok that I keep the null checking in dma_capable()?

No, the generic dma mapping layer is the right place.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Thomas Tai 




On 2020-09-15 11:09 a.m., Christoph Hellwig wrote:

On Tue, Sep 15, 2020 at 10:40:39AM -0400, Thomas Tai wrote:

+++ b/include/linux/dma-direct.h
@@ -62,9 +62,6 @@ static inline bool dma_capable(struct device *dev, dma_addr_t 
addr, size_t size,
   {
dma_addr_t end = addr + size - 1;
   -if (!dev->dma_mask)
-   return false;
-


I am concerned that some drivers may rely on this NULL checking. Would you
think we can keep this checking and use the following WARN_ON_ONCE()?


dma_capable is not a helper for drivers, but just for dma-direct
and related code.  And this patch adds the checks for the three
places how we call into the ->map* methods.



Hi Christoph,
I tried out the suggested changes, and it successfully warned the null 
pointer without panic. I notice that there are some places outside the 
dma-direct, which calls dma_capable().


https://elixir.bootlin.com/linux/v5.9-rc5/source/arch/x86/kernel/amd_gart_64.c#L187

https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/xen/swiotlb-xen.c#L387

Also, if I remove the null checking in dma_capable(), I may run into the 
risk of a null pointer dereference within the function.


@@ -62,9 +62,6 @@ static inline bool dma_capable(struct device *dev, 
dma_addr_t addr, size_t size,

 {
dma_addr_t end = addr + size - 1;

-   if (!dev->dma_mask)
-   return false;
-
if (is_ram && !IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
min(addr, end) < phys_to_dma(dev, PFN_PHYS(min_low_pfn)))
return false;

return end <= min_not_zero(*dev->dma_mask, dev->bus_dma_limit);
^
|
** risk of a null dereference **
}


Given that the WARN_ON_ONCE already did the intended warning, would you 
be ok that I keep the null checking in dma_capable()?


Thank you,
Thomas

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Thomas Tai 




On 2020-09-15 11:09 a.m., Christoph Hellwig wrote:

On Tue, Sep 15, 2020 at 10:40:39AM -0400, Thomas Tai wrote:

+++ b/include/linux/dma-direct.h
@@ -62,9 +62,6 @@ static inline bool dma_capable(struct device *dev, dma_addr_t 
addr, size_t size,
   {
dma_addr_t end = addr + size - 1;
   -if (!dev->dma_mask)
-   return false;
-


I am concerned that some drivers may rely on this NULL checking. Would you
think we can keep this checking and use the following WARN_ON_ONCE()?


dma_capable is not a helper for drivers, but just for dma-direct
and related code.  And this patch adds the checks for the three
places how we call into the ->map* methods.



Ok. That sounds good to me. I will make the suggested changes and run 
some tests before sending out the V2 patch.


Thank you,
Thomas
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Christoph Hellwig 
On Tue, Sep 15, 2020 at 10:40:39AM -0400, Thomas Tai wrote:
>> +++ b/include/linux/dma-direct.h
>> @@ -62,9 +62,6 @@ static inline bool dma_capable(struct device *dev, 
>> dma_addr_t addr, size_t size,
>>   {
>>  dma_addr_t end = addr + size - 1;
>>   -  if (!dev->dma_mask)
>> -return false;
>> -
>
> I am concerned that some drivers may rely on this NULL checking. Would you 
> think we can keep this checking and use the following WARN_ON_ONCE()?

dma_capable is not a helper for drivers, but just for dma-direct
and related code.  And this patch adds the checks for the three
places how we call into the ->map* methods.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Thomas Tai 



On 2020-09-15 10:26 a.m., Christoph Hellwig wrote:

On Tue, Sep 15, 2020 at 10:11:51AM -0400, Thomas Tai wrote:



On 2020-09-15 10:07 a.m., Christoph Hellwig wrote:

On Tue, Sep 15, 2020 at 08:03:14AM -0600, Thomas Tai wrote:

When booting the kernel v5.9-rc4 on a VM, the kernel would panic when
printing a warning message in swiotlb_map(). It is because dev->dma_mask
can potentially be a null pointer. Using the dma_get_mask() macro can
avoid the NULL pointer dereference.


dma_mask must not be zero.  This means drm is calling DMA API functions
on something weird.  This needs to be fixed in the caller.



Thanks, Christoph for your comment. The caller already fixed the null
pointer in the latest v5.9-rc5. I am thinking that if we had used the
dma_get_mask(), the kernel couldn't panic and could properly print out the
warning message.


If we want to solve this something like this patch is probably the
right way:



diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 6e87225600ae35..064870844f06c1 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -62,9 +62,6 @@ static inline bool dma_capable(struct device *dev, dma_addr_t 
addr, size_t size,
  {
dma_addr_t end = addr + size - 1;
  
-	if (!dev->dma_mask)

-   return false;
-


I am concerned that some drivers may rely on this NULL checking. Would 
you think we can keep this checking and use the following WARN_ON_ONCE()?



if (is_ram && !IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
min(addr, end) < phys_to_dma(dev, PFN_PHYS(min_low_pfn)))
return false;
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 0d129421e75fc8..2b01d8f7baf160 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -144,6 +144,10 @@ dma_addr_t dma_map_page_attrs(struct device *dev, struct 
page *page,
dma_addr_t addr;
  
  	BUG_ON(!valid_dma_direction(dir));

+
+   if (WARN_ON_ONCE(!dev->dma_mask))
+   return DMA_MAPPING_ERROR;
+
if (dma_map_direct(dev, ops))
addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
else
@@ -179,6 +183,10 @@ int dma_map_sg_attrs(struct device *dev, struct 
scatterlist *sg, int nents,
int ents;
  
  	BUG_ON(!valid_dma_direction(dir));

+
+   if (WARN_ON_ONCE(!dev->dma_mask))
+   return 0;
+
if (dma_map_direct(dev, ops))
ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
else
@@ -217,6 +225,9 @@ dma_addr_t dma_map_resource(struct device *dev, phys_addr_t 
phys_addr,
if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr
return DMA_MAPPING_ERROR;
  
+	if (WARN_ON_ONCE(!dev->dma_mask))

+   return DMA_MAPPING_ERROR;
+
if (dma_map_direct(dev, ops))
addr = dma_direct_map_resource(dev, phys_addr, size, dir, 
attrs);
else if (ops->map_resource)


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Christoph Hellwig 
On Tue, Sep 15, 2020 at 10:11:51AM -0400, Thomas Tai wrote:
>
>
> On 2020-09-15 10:07 a.m., Christoph Hellwig wrote:
>> On Tue, Sep 15, 2020 at 08:03:14AM -0600, Thomas Tai wrote:
>>> When booting the kernel v5.9-rc4 on a VM, the kernel would panic when
>>> printing a warning message in swiotlb_map(). It is because dev->dma_mask
>>> can potentially be a null pointer. Using the dma_get_mask() macro can
>>> avoid the NULL pointer dereference.
>>
>> dma_mask must not be zero.  This means drm is calling DMA API functions
>> on something weird.  This needs to be fixed in the caller.
>>
>
> Thanks, Christoph for your comment. The caller already fixed the null 
> pointer in the latest v5.9-rc5. I am thinking that if we had used the 
> dma_get_mask(), the kernel couldn't panic and could properly print out the 
> warning message.

If we want to solve this something like this patch is probably the
right way:



diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 6e87225600ae35..064870844f06c1 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -62,9 +62,6 @@ static inline bool dma_capable(struct device *dev, dma_addr_t 
addr, size_t size,
 {
dma_addr_t end = addr + size - 1;
 
-   if (!dev->dma_mask)
-   return false;
-
if (is_ram && !IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
min(addr, end) < phys_to_dma(dev, PFN_PHYS(min_low_pfn)))
return false;
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 0d129421e75fc8..2b01d8f7baf160 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -144,6 +144,10 @@ dma_addr_t dma_map_page_attrs(struct device *dev, struct 
page *page,
dma_addr_t addr;
 
BUG_ON(!valid_dma_direction(dir));
+
+   if (WARN_ON_ONCE(!dev->dma_mask))
+   return DMA_MAPPING_ERROR;
+
if (dma_map_direct(dev, ops))
addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
else
@@ -179,6 +183,10 @@ int dma_map_sg_attrs(struct device *dev, struct 
scatterlist *sg, int nents,
int ents;
 
BUG_ON(!valid_dma_direction(dir));
+
+   if (WARN_ON_ONCE(!dev->dma_mask))
+   return 0;
+
if (dma_map_direct(dev, ops))
ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
else
@@ -217,6 +225,9 @@ dma_addr_t dma_map_resource(struct device *dev, phys_addr_t 
phys_addr,
if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr
return DMA_MAPPING_ERROR;
 
+   if (WARN_ON_ONCE(!dev->dma_mask))
+   return DMA_MAPPING_ERROR;
+
if (dma_map_direct(dev, ops))
addr = dma_direct_map_resource(dev, phys_addr, size, dir, 
attrs);
else if (ops->map_resource)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Thomas Tai 




On 2020-09-15 10:07 a.m., Christoph Hellwig wrote:

On Tue, Sep 15, 2020 at 08:03:14AM -0600, Thomas Tai wrote:

When booting the kernel v5.9-rc4 on a VM, the kernel would panic when
printing a warning message in swiotlb_map(). It is because dev->dma_mask
can potentially be a null pointer. Using the dma_get_mask() macro can
avoid the NULL pointer dereference.


dma_mask must not be zero.  This means drm is calling DMA API functions
on something weird.  This needs to be fixed in the caller.



Thanks, Christoph for your comment. The caller already fixed the null 
pointer in the latest v5.9-rc5. I am thinking that if we had used the 
dma_get_mask(), the kernel couldn't panic and could properly print out 
the warning message.


Thomas
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Christoph Hellwig 
On Tue, Sep 15, 2020 at 04:07:19PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 08:03:14AM -0600, Thomas Tai wrote:
> > When booting the kernel v5.9-rc4 on a VM, the kernel would panic when
> > printing a warning message in swiotlb_map(). It is because dev->dma_mask
> > can potentially be a null pointer. Using the dma_get_mask() macro can
> > avoid the NULL pointer dereference.
> 
> dma_mask must not be zero.  This means drm is calling DMA API functions
> on something weird.  This needs to be fixed in the caller.

s/zero/NULL/, but the point stands.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-direct: Fix potential NULL pointer dereference

2020-09-15 Thread Christoph Hellwig 
On Tue, Sep 15, 2020 at 08:03:14AM -0600, Thomas Tai wrote:
> When booting the kernel v5.9-rc4 on a VM, the kernel would panic when
> printing a warning message in swiotlb_map(). It is because dev->dma_mask
> can potentially be a null pointer. Using the dma_get_mask() macro can
> avoid the NULL pointer dereference.

dma_mask must not be zero.  This means drm is calling DMA API functions
on something weird.  This needs to be fixed in the caller.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu