Re: [PATCH] dax: Fix potential uaf in __dax_pmem_probe()
Hi Jianglei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.19-rc4 next-20220629]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/intel-lab-lkp/linux/commits/Jianglei-Nie/dax-Fix-potential-uaf-in-__dax_pmem_probe/20220629-152544
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
941e3e7912696b9fbe3586083a7c2e102cee7a87
config: x86_64-rhel-8.3
(https://download.01.org/0day-ci/archive/20220629/202206292354.bfjgfb0c-...@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
#
https://github.com/intel-lab-lkp/linux/commit/3b977a761f194fc61bdd0f6e97e1863ff32a04c5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Jianglei-Nie/dax-Fix-potential-uaf-in-__dax_pmem_probe/20220629-152544
git checkout 3b977a761f194fc61bdd0f6e97e1863ff32a04c5
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/dax/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
All error/warnings (new ones prefixed by >>):
drivers/dax/pmem.c: In function '__dax_pmem_probe':
>> drivers/dax/pmem.c:70:41: error: expected ')' before ';' token
70 | return ERR_PTR((dev_dax);
| ~ ^
| )
>> drivers/dax/pmem.c:70:32: warning: passing argument 1 of 'ERR_PTR' makes
>> integer from pointer without a cast [-Wint-conversion]
70 | return ERR_PTR((dev_dax);
|^
||
|struct dev_dax *
In file included from include/linux/container_of.h:6,
from include/linux/list.h:5,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:55,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:6,
from include/linux/mm.h:7,
from include/linux/memremap.h:5,
from drivers/dax/pmem.c:3:
include/linux/err.h:24:48: note: expected 'long int' but argument is of type
'struct dev_dax *'
24 | static inline void * __must_check ERR_PTR(long error)
| ~^
>> drivers/dax/pmem.c:75:24: error: expected ';' before '}' token
75 | return dev_dax;
|^
|;
76 | }
| ~
drivers/dax/pmem.c:76:1: error: control reaches end of non-void function
[-Werror=return-type]
76 | }
| ^
cc1: some warnings being treated as errors
vim +70 drivers/dax/pmem.c
9
10 static struct dev_dax *__dax_pmem_probe(struct device *dev)
11 {
12 struct range range;
13 int rc, id, region_id;
14 resource_size_t offset;
15 struct nd_pfn_sb *pfn_sb;
16 struct dev_dax *dev_dax;
17 struct dev_dax_data data;
18 struct nd_namespace_io *nsio;
19 struct dax_region *dax_region;
20 struct dev_pagemap pgmap = { };
21 struct nd_namespace_common *ndns;
22 struct nd_dax *nd_dax = to_nd_dax(dev);
23 struct nd_pfn *nd_pfn = _dax->nd_pfn;
24 struct nd_region *nd_region = to_nd_region(dev->parent);
25
26 ndns = nvdimm_namespace_common_probe(dev);
27 if (IS_ERR(ndns))
28 return ERR_CAST(ndns);
29
30 /* parse the 'pfn' info block via ->rw_bytes */
31 rc = devm_namespace_enable(dev, ndns, nd_info_block_reserve());
32 if (rc)
33 return ERR_PTR(rc);
34 rc = nvdimm_setup_pfn(nd_pfn, );
35 if (rc)
36 return ERR_PTR(rc);
37 devm_namespace_disable(dev, ndns);
38
39 /* reserve the metadata area, device-dax will reserve the data
*/
40 pfn_sb = nd_pfn->pfn_sb;
41 offset = le64_to_cpu(pfn_sb->dataoff);
42 nsio = to_nd_namespace_io(>dev);
43 if (!devm_request_mem_region(dev, nsio->res.start, offset,
44 dev_name(>dev))) {
45 dev_warn(dev, "could not reserve metadata\n");
46 return ERR_PTR(-EBUSY);
47 }
48
49 rc = sscanf(dev_name(>dev), "namespace%d.%d", _id,
);
50
[PATCH] dax: Fix potential uaf in __dax_pmem_probe()
__dax_pmem_probe() allocates a memory chunk from dax_region with alloc_dax_region(). alloc_dax_region() increases the refcount for dax_region and uses devm_add_action_or_reset() to make the parent dev manage the dax_region. The dax_region will be used if the parent dev is destroyed. Then the function calls devm_create_dev_dax() to make child dev_dax instances own the lifetime of the dax_region. devm_create_dev_dax() calls devm_add_action_or_reset(dax_region->dev, unregister_dev_dax, dev); to make the child dev_dax manage the dax_region and register the destroy function "unregister_dev_dax".The devm_create_dev_dax() increases the refcount for dax_region when the function is successfully executed. But when some error occurs, devm_create_dev_dax() may return ERR_PTR before increasing the refcount for dax_region. In these cases, the call for dax_region_put() will decrease the ref count for dax_region and trigger dax_region_free(), which will execute kfree(dax_region). When the parent dev is destroyed, the registered destroy function "unregister_dev_dax" will be triggered and calls dax_region_free(), which will use the freed dax_region, leading to a use after free bug. We should check the return value of devm_create_dev_dax(). If it returns ERR_PTR, we should return this function with ERR_PTR. Signed-off-by: Jianglei Nie --- drivers/dax/pmem.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c index f050ea78bb83..d5c8bd546ee9 100644 --- a/drivers/dax/pmem.c +++ b/drivers/dax/pmem.c @@ -66,6 +66,8 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) .size = range_len(), }; dev_dax = devm_create_dev_dax(); + if (IS_ERR(dev_dax)) + return ERR_PTR((dev_dax); /* child dev_dax instances now own the lifetime of the dax_region */ dax_region_put(dax_region); -- 2.25.1
[PATCH] dax: Fix potential uaf in __dax_pmem_probe()
__dax_pmem_probe() allocates a memory chunk from dax_region with alloc_dax_region(). alloc_dax_region() increases the refcount for dax_region and uses devm_add_action_or_reset() to make the parent dev manage the dax_region. The dax_region will be used if the parent dev is destroyed. Then the function calls devm_create_dev_dax() to make child dev_dax instances own the lifetime of the dax_region. devm_create_dev_dax() calls devm_add_action_or_reset(dax_region->dev, unregister_dev_dax, dev); to make the child dev_dax manage the dax_region and register the destroy function "unregister_dev_dax".The devm_create_dev_dax() increases the refcount for dax_region when the function is successfully executed. But when some error occurs, devm_create_dev_dax() may return ERR_PTR before increasing the refcount for dax_region. In these cases, the call for dax_region_put() will decrease the ref count for dax_region and trigger dax_region_free(), which will execute kfree(dax_region). When the parent dev is destroyed, the registered destroy function "unregister_dev_dax" will be triggered and calls dax_region_free(), which will use the freed dax_region, leading to a use after free bug. We should check the return value of devm_create_dev_dax(). If it returns ERR_PTR, we should return this function with ERR_PTR. Signed-off-by: Jianglei Nie --- drivers/dax/pmem.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c index f050ea78bb83..d5c8bd546ee9 100644 --- a/drivers/dax/pmem.c +++ b/drivers/dax/pmem.c @@ -66,6 +66,8 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) .size = range_len(), }; dev_dax = devm_create_dev_dax(); + if (IS_ERR(dev_dax)) + return ERR_PTR((dev_dax); /* child dev_dax instances now own the lifetime of the dax_region */ dax_region_put(dax_region); -- 2.25.1
