[PATCH] dax: Fix potential uaf in __dax_pmem_probe()

[PATCH] dax: Fix potential uaf in __dax_pmem_probe()

[Jianglei Nie]

__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 <niejianglei2021@163.com>
---
 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(&range),
 	};
 	dev_dax = devm_create_dev_dax(&data);
+	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

Re: [PATCH] dax: Fix potential uaf in __dax_pmem_probe()

[kernel test robot]

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-lkp@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 <lkp@intel.com>

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 = &nd_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, &pgmap);
    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(&ndns->dev);
    43		if (!devm_request_mem_region(dev, nsio->res.start, offset,
    44					dev_name(&ndns->dev))) {
    45			dev_warn(dev, "could not reserve metadata\n");
    46			return ERR_PTR(-EBUSY);
    47		}
    48	
    49		rc = sscanf(dev_name(&ndns->dev), "namespace%d.%d", &region_id, &id);
    50		if (rc != 2)
    51			return ERR_PTR(-EINVAL);
    52	
    53		/* adjust the dax_region range to the start of data */
    54		range = pgmap.range;
    55		range.start += offset;
    56		dax_region = alloc_dax_region(dev, region_id, &range,
    57				nd_region->target_node, le32_to_cpu(pfn_sb->align),
    58				IORESOURCE_DAX_STATIC);
    59		if (!dax_region)
    60			return ERR_PTR(-ENOMEM);
    61	
    62		data = (struct dev_dax_data) {
    63			.dax_region = dax_region,
    64			.id = id,
    65			.pgmap = &pgmap,
    66			.size = range_len(&range),
    67		};
    68		dev_dax = devm_create_dev_dax(&data);
    69		if (IS_ERR(dev_dax))
  > 70			return ERR_PTR((dev_dax);
    71	
    72		/* child dev_dax instances now own the lifetime of the dax_region */
    73		dax_region_put(dax_region);
    74	
  > 75		return dev_dax;
    76	}
    77	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH] dax: Fix potential uaf in __dax_pmem_probe()

[Jianglei Nie]

__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 <niejianglei2021@163.com>
---
 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(&range),
 	};
 	dev_dax = devm_create_dev_dax(&data);
+	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