[POSSIBLE BUG] iommu/io-pgtable-arm: possible dereferencing of NULL pointer

[POSSIBLE BUG] iommu/io-pgtable-arm: possible dereferencing of NULL pointer

[Subkhankulov Rustam]

Version: 5-19-rc6

In function '__arm_lpae_alloc_pages' pointer 'dev' is compared with 
NULL at [drivers/iommu/io-pgtable-arm.c: 203]. This means that the 
pointer can be NULL.

-----------------------------------------------------------------------
203 	p = alloc_pages_node(dev ? dev_to_node(dev) : NUMA_NO_NODE,
204 			     gfp | __GFP_ZERO, order);
-----------------------------------------------------------------------

Then, if cfg->coherent_walk == 0 at [drivers/iommu/io-pgtable-arm.c: 
209], function 'dma_map_single', which is defined as 
'dma_map_single_attrs', is called and pointer dev is passed as 
first parameter.

-----------------------------------------------------------------------
209 	if (!cfg->coherent_walk) {
208 		dma = dma_map_single(dev, pages, size, DMA_TO_DEVICE);
-----------------------------------------------------------------------

Therefore, pointer 'dev' passed to function 'dev_driver_string' 
in macro 'dev_WARN_ONCE' at [include/linux/dma-mapping.h: 326], 
where it is dereferenced at [drivers/base/core.c: 2091].

-----------------------------------------------------------------------
2083	const char *dev_driver_string(const struct device *dev)
2084	{
2085		struct device_driver *drv;
2086
---
2091		drv = READ_ONCE(dev->driver);
-----------------------------------------------------------------------

Thus, if it is possible that 'dev' is null at the same time 
that flag 'coherent_walk' is 0, then NULL pointer will be 
dereferenced.

Should we somehow avoid NULL pointer dereference or is this 
situation impossible and we should remove comparison with NULL?

Found by Linux Verification Center (linuxtesting.org) with SVACE.

regards,
Rustam Subkhankulov

Re: [POSSIBLE BUG] iommu/io-pgtable-arm: possible dereferencing of NULL pointer

[Will Deacon]

On Mon, Jul 18, 2022 at 12:20:06PM +0300, Subkhankulov Rustam wrote:
> Version: 5-19-rc6
> 
> In function '__arm_lpae_alloc_pages' pointer 'dev' is compared with 
> NULL at [drivers/iommu/io-pgtable-arm.c: 203]. This means that the 
> pointer can be NULL.
> 
> -----------------------------------------------------------------------
> 203 	p = alloc_pages_node(dev ? dev_to_node(dev) : NUMA_NO_NODE,
> 204 			     gfp | __GFP_ZERO, order);
> -----------------------------------------------------------------------
> 
> Then, if cfg->coherent_walk == 0 at [drivers/iommu/io-pgtable-arm.c: 
> 209], function 'dma_map_single', which is defined as 
> 'dma_map_single_attrs', is called and pointer dev is passed as 
> first parameter.
> 
> -----------------------------------------------------------------------
> 209 	if (!cfg->coherent_walk) {
> 208 		dma = dma_map_single(dev, pages, size, DMA_TO_DEVICE);
> -----------------------------------------------------------------------
> 
> Therefore, pointer 'dev' passed to function 'dev_driver_string' 
> in macro 'dev_WARN_ONCE' at [include/linux/dma-mapping.h: 326], 
> where it is dereferenced at [drivers/base/core.c: 2091].
> 
> -----------------------------------------------------------------------
> 2083	const char *dev_driver_string(const struct device *dev)
> 2084	{
> 2085		struct device_driver *drv;
> 2086
> ---
> 2091		drv = READ_ONCE(dev->driver);
> -----------------------------------------------------------------------
> 
> Thus, if it is possible that 'dev' is null at the same time 
> that flag 'coherent_walk' is 0, then NULL pointer will be 
> dereferenced.
> 
> Should we somehow avoid NULL pointer dereference or is this 
> situation impossible and we should remove comparison with NULL?

I think 'dev' is only null in the case of the selftest initcall
(see arm_lpae_do_selftests()), and 'coherent_walk' is always true there.

Will

Re: [POSSIBLE BUG] iommu/io-pgtable-arm: possible dereferencing of NULL pointer

[Robin Murphy]

On 2022-07-19 18:36, Will Deacon wrote:
> On Mon, Jul 18, 2022 at 12:20:06PM +0300, Subkhankulov Rustam wrote:
>> Version: 5-19-rc6
>>
>> In function '__arm_lpae_alloc_pages' pointer 'dev' is compared with
>> NULL at [drivers/iommu/io-pgtable-arm.c: 203]. This means that the
>> pointer can be NULL.
>>
>> -----------------------------------------------------------------------
>> 203 	p = alloc_pages_node(dev ? dev_to_node(dev) : NUMA_NO_NODE,
>> 204 			     gfp | __GFP_ZERO, order);
>> -----------------------------------------------------------------------
>>
>> Then, if cfg->coherent_walk == 0 at [drivers/iommu/io-pgtable-arm.c:
>> 209], function 'dma_map_single', which is defined as
>> 'dma_map_single_attrs', is called and pointer dev is passed as
>> first parameter.
>>
>> -----------------------------------------------------------------------
>> 209 	if (!cfg->coherent_walk) {
>> 208 		dma = dma_map_single(dev, pages, size, DMA_TO_DEVICE);
>> -----------------------------------------------------------------------
>>
>> Therefore, pointer 'dev' passed to function 'dev_driver_string'
>> in macro 'dev_WARN_ONCE' at [include/linux/dma-mapping.h: 326],
>> where it is dereferenced at [drivers/base/core.c: 2091].
>>
>> -----------------------------------------------------------------------
>> 2083	const char *dev_driver_string(const struct device *dev)
>> 2084	{
>> 2085		struct device_driver *drv;
>> 2086
>> ---
>> 2091		drv = READ_ONCE(dev->driver);
>> -----------------------------------------------------------------------
>>
>> Thus, if it is possible that 'dev' is null at the same time
>> that flag 'coherent_walk' is 0, then NULL pointer will be
>> dereferenced.
>>
>> Should we somehow avoid NULL pointer dereference or is this
>> situation impossible and we should remove comparison with NULL?
> 
> I think 'dev' is only null in the case of the selftest initcall
> (see arm_lpae_do_selftests()), and 'coherent_walk' is always true there.

Indeed, the intent is that cfg->iommu_dev == NULL is a special case for 
the selftest, which must always claim coherency as well for this reason. 
I suppose we could add an explicit assertion along those lines in 
alloc_pgtable if anyone really thinks it matters.

Cheers,
Robin.

Re: [POSSIBLE BUG] iommu/io-pgtable-arm: possible dereferencing of NULL pointer

[Alexey Khoroshilov]

On 01.08.2022 14:06, Robin Murphy wrote:
> On 2022-07-19 18:36, Will Deacon wrote:
>> On Mon, Jul 18, 2022 at 12:20:06PM +0300, Subkhankulov Rustam wrote:
>>> Version: 5-19-rc6
>>>
>>> In function '__arm_lpae_alloc_pages' pointer 'dev' is compared with
>>> NULL at [drivers/iommu/io-pgtable-arm.c: 203]. This means that the
>>> pointer can be NULL.
>>>
>>> -----------------------------------------------------------------------
>>> 203     p = alloc_pages_node(dev ? dev_to_node(dev) : NUMA_NO_NODE,
>>> 204                  gfp | __GFP_ZERO, order);
>>> -----------------------------------------------------------------------
>>>
>>> Then, if cfg->coherent_walk == 0 at [drivers/iommu/io-pgtable-arm.c:
>>> 209], function 'dma_map_single', which is defined as
>>> 'dma_map_single_attrs', is called and pointer dev is passed as
>>> first parameter.
>>>
>>> -----------------------------------------------------------------------
>>> 209     if (!cfg->coherent_walk) {
>>> 208         dma = dma_map_single(dev, pages, size, DMA_TO_DEVICE);
>>> -----------------------------------------------------------------------
>>>
>>> Therefore, pointer 'dev' passed to function 'dev_driver_string'
>>> in macro 'dev_WARN_ONCE' at [include/linux/dma-mapping.h: 326],
>>> where it is dereferenced at [drivers/base/core.c: 2091].
>>>
>>> -----------------------------------------------------------------------
>>> 2083    const char *dev_driver_string(const struct device *dev)
>>> 2084    {
>>> 2085        struct device_driver *drv;
>>> 2086
>>> ---
>>> 2091        drv = READ_ONCE(dev->driver);
>>> -----------------------------------------------------------------------
>>>
>>> Thus, if it is possible that 'dev' is null at the same time
>>> that flag 'coherent_walk' is 0, then NULL pointer will be
>>> dereferenced.
>>>
>>> Should we somehow avoid NULL pointer dereference or is this
>>> situation impossible and we should remove comparison with NULL?
>>
>> I think 'dev' is only null in the case of the selftest initcall
>> (see arm_lpae_do_selftests()), and 'coherent_walk' is always true there.
> 
> Indeed, the intent is that cfg->iommu_dev == NULL is a special case for
> the selftest, which must always claim coherency as well for this reason.
> I suppose we could add an explicit assertion along those lines in
> alloc_pgtable if anyone really thinks it matters.

Yes, we believe it make sense. It will help to document the intention
and to avoid future questions.

Thank you,
Alexey