| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <67c7900c976ed_24b64294c9@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 4 Mar 2025 15:43:08 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Li Zhijian <lizhijian@...itsu.com>, <linux-kernel@...r.kernel.org>
CC: Andrew Morton <akpm@...ux-foundation.org>, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com>, <ilpo.jarvinen@...ux.intel.com>, "Mika
Westerberg" <mika.westerberg@...ux.intel.com>, Bjorn Helgaas
<bhelgaas@...gle.com>, Ying Huang <huang.ying.caritas@...il.com>, "Dan
Williams" <dan.j.williams@...el.com>, Jonathan Cameron
<Jonathan.Cameron@...wei.com>, <linux-cxl@...r.kernel.org>, Li Zhijian
<lizhijian@...itsu.com>
Subject: Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
Li Zhijian wrote:
> The leak is detected by the kernel memory leak detector (`kmemleak`)
> following a `cxl create-region` failure:
>
> cxl_acpi ACPI0017:00: decoder0.0: created region2
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> Signed-off-by: Li Zhijian <lizhijian@...itsu.com>
> ---
> kernel/resource.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 12004452d999..aa0b1da143eb 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base,
> devres_free(dr);
> } else if (dev)
> devm_release_action(dev, remove_free_mem_region, res);
> + else
> + free_resource(res);
This looks deceptively correct, but if the __insert_resource() call
succeeded above then this needs to optionally be paired with
remove_resource().
I think this function needs a rethink because mixing the devres, devm,
and alloc_resource() failure cases makes mistakes like this hard to see.
Here is a replacement proposal, only compile-tested:
-- >8 --
>From a01a28304e547da1f6287eecf3aeb0ebc6f48e2b Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@...el.com>
Date: Tue, 4 Mar 2025 15:12:19 -0800
Subject: [PATCH] resource: Fix resource leak in get_free_mem_region()
Li reports a kmemleak detection in get_free_mem_region() error unwind
path:
cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
__kmalloc_cache_noprof+0x28c/0x350
get_free_mem_region+0x45/0x380
alloc_free_mem_region+0x1d/0x30
size_store+0x180/0x290 [cxl_core]
kernfs_fop_write_iter+0x13f/0x1e0
vfs_write+0x37c/0x540
ksys_write+0x68/0xe0
do_syscall_64+0x6e/0x190
entry_SYSCALL_64_after_hwframe+0x76/0x7e
It turns out it not only leaks memory, also fails to unwind changes to
the resource tree (@base, usually iomem_resource).
Fix this by consolidating the devres and devm paths into just devres,
and move those details to a wrapper function. So now
__get_free_mem_region() only needs to worry about alloc_resource()
unwinding, and the devres failure path is resolved before touching the
resource tree.
Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
Reported-by: Li Zhijian <lizhijian@...itsu.com>
Closes: http://lore.kernel.org/20250304043415.610286-1-lizhijian@fujitsu.com
Signed-off-by: Dan Williams <dan.j.williams@...el.com>
---
kernel/resource.c | 105 ++++++++++++++++++++++++++--------------------
1 file changed, 60 insertions(+), 45 deletions(-)
diff --git a/kernel/resource.c b/kernel/resource.c
index 12004452d999..80d10714cb38 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -172,6 +172,8 @@ static void free_resource(struct resource *res)
kfree(res);
}
+DEFINE_FREE(free_resource, struct resource *, if (_T) free_resource(_T))
+
static struct resource *alloc_resource(gfp_t flags)
{
return kzalloc(sizeof(struct resource), flags);
@@ -1631,17 +1633,29 @@ void devm_release_resource(struct device *dev, struct resource *new)
}
EXPORT_SYMBOL(devm_release_resource);
+/*
+ * The GFR_REQUEST_REGION case performs a request_region() to be paired
+ * with release_region(). The alloc_free_mem_region() path performs
+ * insert_resource() to be paired with {remove,free}_resource(). The
+ * @res member differentiates the 2 cases.
+ */
struct region_devres {
struct resource *parent;
resource_size_t start;
resource_size_t n;
+ struct resource *res;
};
static void devm_region_release(struct device *dev, void *res)
{
struct region_devres *this = res;
- __release_region(this->parent, this->start, this->n);
+ if (!this->res) {
+ __release_region(this->parent, this->start, this->n);
+ } else {
+ remove_resource(this->res);
+ free_resource(this->res);
+ }
}
static int devm_region_match(struct device *dev, void *res, void *match_data)
@@ -1908,43 +1922,19 @@ static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
return addr + size;
}
-static void remove_free_mem_region(void *_res)
-{
- struct resource *res = _res;
-
- if (res->parent)
- remove_resource(res);
- free_resource(res);
-}
-
static struct resource *
-get_free_mem_region(struct device *dev, struct resource *base,
- resource_size_t size, const unsigned long align,
- const char *name, const unsigned long desc,
- const unsigned long flags)
+__get_free_mem_region(struct resource *base, resource_size_t size,
+ const unsigned long align, const char *name,
+ const unsigned long desc, const unsigned long flags)
{
resource_size_t addr;
- struct resource *res;
- struct region_devres *dr = NULL;
size = ALIGN(size, align);
- res = alloc_resource(GFP_KERNEL);
+ struct resource *res __free(free_resource) = alloc_resource(GFP_KERNEL);
if (!res)
return ERR_PTR(-ENOMEM);
- if (dev && (flags & GFR_REQUEST_REGION)) {
- dr = devres_alloc(devm_region_release,
- sizeof(struct region_devres), GFP_KERNEL);
- if (!dr) {
- free_resource(res);
- return ERR_PTR(-ENOMEM);
- }
- } else if (dev) {
- if (devm_add_action_or_reset(dev, remove_free_mem_region, res))
- return ERR_PTR(-ENOMEM);
- }
-
write_lock(&resource_lock);
for (addr = gfr_start(base, size, align, flags);
gfr_continue(base, addr, align, flags);
@@ -1958,17 +1948,9 @@ get_free_mem_region(struct device *dev, struct resource *base,
size, name, 0))
break;
- if (dev) {
- dr->parent = &iomem_resource;
- dr->start = addr;
- dr->n = size;
- devres_add(dev, dr);
- }
-
res->desc = desc;
write_unlock(&resource_lock);
-
/*
* A driver is claiming this region so revoke any
* mappings.
@@ -1985,25 +1967,58 @@ get_free_mem_region(struct device *dev, struct resource *base,
* Only succeed if the resource hosts an exclusive
* range after the insert
*/
- if (__insert_resource(base, res) || res->child)
+ if (__insert_resource(base, res))
+ break;
+ if (res->child) {
+ remove_resource(res);
break;
+ }
write_unlock(&resource_lock);
}
- return res;
+ return no_free_ptr(res);
}
write_unlock(&resource_lock);
- if (flags & GFR_REQUEST_REGION) {
- free_resource(res);
- devres_free(dr);
- } else if (dev)
- devm_release_action(dev, remove_free_mem_region, res);
-
return ERR_PTR(-ERANGE);
}
+static struct resource *
+get_free_mem_region(struct device *dev, struct resource *base,
+ resource_size_t size, const unsigned long align,
+ const char *name, const unsigned long desc,
+ const unsigned long flags)
+{
+
+ struct region_devres *dr __free(kfree) = NULL;
+ struct resource *res;
+
+ if (dev) {
+ dr = devres_alloc(devm_region_release,
+ sizeof(struct region_devres), GFP_KERNEL);
+ if (!dr)
+ return ERR_PTR(-ENOMEM);
+ }
+
+ res = __get_free_mem_region(base, size, align, name, desc, flags);
+
+ if (IS_ERR(res) || !dr)
+ return res;
+
+ dr->parent = base;
+ dr->start = res->start;
+ dr->n = resource_size(res);
+
+ /* See 'struct region_devres' definition for details */
+ if ((flags & GFR_REQUEST_REGION) == 0)
+ dr->res = res;
+
+ devres_add(dev, no_free_ptr(dr));
+
+ return res;
+}
+
/**
* devm_request_free_mem_region - find free region for device private memory
*
--
2.48.1
Powered by blists - more mailing lists