Messages in this thread | | | Subject | Re: [POSSIBLE BUG] iommu/io-pgtable-arm: possible dereferencing of NULL pointer | From | Alexey Khoroshilov <> | Date | Tue, 2 Aug 2022 01:33:20 +0300 |
| |
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
| |