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

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

2020-09-15 Thread Thomas Tai 
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.

Fixes: d323bb44e4d2 ("drm/virtio: Call the right shmem helpers")

[drm] Initialized virtio_gpu 0.1.0 0 for virtio0 on minor 0
 BUG: kernel NULL pointer dereference, address: 
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x) - not-present page
 PGD 0 P4D 0
 Oops:  [#1] SMP PTI
 CPU: 1 PID: 331 Comm: systemd-udevd Not tainted 5.9.0-rc4 #1
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
 BIOS 1.13.0-1ubuntu1 04/01/2014
 RIP: 0010:swiotlb_map+0x1ac/0x200
 Code: e8 d9 fc ff ff 80 3d 92 ee 4c 01 00 75 51 49 8b 84 24 48 02 00 00
 4d 8b 6c 24 50 c6 05 7c ee 4c 01 01 4d 8b bc 24 58 02 00 00 <4c> 8b 30
 4d 85 ed 75 04 4d 8b 2c 24 4c 89 e7 e8 10 6b 4f 00 4d 89
 RSP: 0018:9f96801af6f8 EFLAGS: 00010246
 RAX:  RBX: 1000 RCX: 0080
 RDX: 007f RSI: 0202 RDI: 0202
 RBP: 9f96801af748 R08:  R09: 0020
 R10:  R11: 8fabfffa3000 R12: 8faad02c7810
 R13:  R14: 0020 R15: 
 FS:  7fabc63588c0() GS:8fabf7c8()
 knlGS:
 CS:  0010 DS:  ES:  CR0: 80050033
 CR2:  CR3: 000151496005 CR4: 00370ee0
 DR0:  DR1:  DR2: 
 DR3:  DR6: fffe0ff0 DR7: 0400
 Call Trace:
  dma_direct_map_sg+0x124/0x210
  dma_map_sg_attrs+0x32/0x50
  drm_gem_shmem_get_pages_sgt+0x6a/0x90 [drm]
  virtio_gpu_object_create+0x140/0x2f0 [virtio_gpu]
  ? ww_mutex_unlock+0x26/0x30
  virtio_gpu_mode_dumb_create+0xab/0x160 [virtio_gpu]
  drm_mode_create_dumb+0x82/0x90 [drm]
  drm_client_framebuffer_create+0xaa/0x200 [drm]
  drm_fb_helper_generic_probe+0x59/0x150 [drm_kms_helper]
  drm_fb_helper_single_fb_probe+0x29e/0x3e0 [drm_kms_helper]
  __drm_fb_helper_initial_config_and_unlock+0x41/0xd0 [drm_kms_helper]
  drm_fbdev_client_hotplug+0xe6/0x1a0 [drm_kms_helper]
  drm_fbdev_generic_setup+0xaf/0x170 [drm_kms_helper]
  virtio_gpu_probe+0xea/0x100 [virtio_gpu]
  virtio_dev_probe+0x14b/0x1e0 [virtio]
  really_probe+0x1db/0x440
  driver_probe_device+0xe9/0x160
  device_driver_attach+0x5d/0x70
  __driver_attach+0x8f/0x150
  ? device_driver_attach+0x70/0x70
  bus_for_each_dev+0x7e/0xc0
  driver_attach+0x1e/0x20
  bus_add_driver+0x152/0x1f0
  driver_register+0x74/0xd0
  ? 0xc0529000
  register_virtio_driver+0x20/0x30 [virtio]
  virtio_gpu_driver_init+0x15/0x1000 [virtio_gpu]
  do_one_initcall+0x4a/0x1fa
  ? _cond_resched+0x19/0x30
  ? kmem_cache_alloc_trace+0x16b/0x2e0
  do_init_module+0x62/0x240
  load_module+0xe0e/0x1100
  ? security_kernel_post_read_file+0x5c/0x70
  __do_sys_finit_module+0xbe/0x120
  ? __do_sys_finit_module+0xbe/0x120
  __x64_sys_finit_module+0x1a/0x20
  do_syscall_64+0x38/0x50
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Signed-off-by: Thomas Tai 
---
 include/linux/dma-direct.h | 2 +-
 kernel/dma/swiotlb.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 6e87225..7556067 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -168,7 +168,7 @@ static inline dma_addr_t dma_direct_map_page(struct device 
*dev,
 
dev_WARN_ONCE(dev, 1,
 "DMA addr %pad+%zu overflow (mask %llx, bus limit 
%llx).\n",
-_addr, size, *dev->dma_mask, 
dev->bus_dma_limit);
+_addr, size, dma_get_mask(dev), 
dev->bus_dma_limit);
return DMA_MAPPING_ERROR;
}
 
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c19379fa..aa7727b 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -682,7 +682,7 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t 
paddr, size_t size,
attrs | DMA_ATTR_SKIP_CPU_SYNC);
dev_WARN_ONCE(dev, 1,
"swiotlb addr %pad+%zu overflow (mask %llx, bus limit 
%llx).\n",
-   _addr, size, *dev->dma_mask, dev->bus_dma_limit);
+   _addr, size, dma_get_mask(dev), dev->bus_dma_limit);
return DMA_MAPPING_ERROR;
}
 
-- 
1.8.3.1

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