[PATCH v3] debugobjects: Fix inconsistent return handling and potential ERR_PTR dereference

Haotian Zhang posted 1 patch 2 months, 3 weeks ago
lib/debugobjects.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
[PATCH v3] debugobjects: Fix inconsistent return handling and potential ERR_PTR dereference
Posted by Haotian Zhang 2 months, 3 weeks ago
The lookup_object_or_alloc() function can return NULL on memory
allocation failure, while returning an error pointer for other errors.
Call sites such as __debug_object_init() and debug_object_activate()
only check for errors using IS_ERR(), which does not evaluate to true
for a NULL pointer. This can lead to a NULL pointer dereference if
memory allocation fails.

To fix this, modify lookup_object_or_alloc() to consistently return
ERR_PTR(-ENOMEM) on allocation failure. This ensures that all error
paths return a valid error pointer that can be caught by IS_ERR().
Update all three call sites (__debug_object_init, debug_object_activate,
and debug_object_assert_init) to use IS_ERR() for error checking and
handle -ENOMEM by calling debug_objects_oom(), thus preventing the
potential bug and making the error handling robust and consistent.

Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")
Suggested-by: Kuan-Wei Chiu <visitorckw@gmail.com>
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
Changes in v3:
  -Reword commit message to clearly describe the NULL pointer
   dereference bug, as suggested by Kuan-Wei Chiu.
  -Change subject prefix to "debugobjects:".
---
Changes in v2:
  -Make error handling consistent across all call sites as suggested
   by Kuan-Wei Chiu
---
 lib/debugobjects.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 7f50c4480a4e..d2d1979e2d12 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -691,7 +691,7 @@ static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket
 
 	/* Out of memory. Do the cleanup outside of the locked region */
 	debug_objects_enabled = false;
-	return NULL;
+	return ERR_PTR(-ENOMEM);
 }
 
 static void debug_objects_fill_pool(void)
@@ -741,9 +741,10 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
 	raw_spin_lock_irqsave(&db->lock, flags);
 
 	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
-	if (unlikely(!obj)) {
+	if (IS_ERR(obj)) {
 		raw_spin_unlock_irqrestore(&db->lock, flags);
-		debug_objects_oom();
+		if (PTR_ERR(obj) == -ENOMEM)
+			debug_objects_oom();
 		return;
 	}
 
@@ -818,11 +819,13 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
 	raw_spin_lock_irqsave(&db->lock, flags);
 
 	obj = lookup_object_or_alloc(addr, db, descr, false, true);
-	if (unlikely(!obj)) {
-		raw_spin_unlock_irqrestore(&db->lock, flags);
-		debug_objects_oom();
-		return 0;
-	} else if (likely(!IS_ERR(obj))) {
+	if (IS_ERR(obj)) {
+		if (PTR_ERR(obj) == -ENOMEM) {
+			raw_spin_unlock_irqrestore(&db->lock, flags);
+			debug_objects_oom();
+			return 0;
+		}
+	} else {
 		switch (obj->state) {
 		case ODEBUG_STATE_ACTIVE:
 		case ODEBUG_STATE_DESTROYED:
@@ -1007,11 +1010,11 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
 	raw_spin_lock_irqsave(&db->lock, flags);
 	obj = lookup_object_or_alloc(addr, db, descr, false, true);
 	raw_spin_unlock_irqrestore(&db->lock, flags);
-	if (likely(!IS_ERR_OR_NULL(obj)))
+	if (!IS_ERR(obj))
 		return;
 
 	/* If NULL the allocation has hit OOM */
-	if (!obj) {
+	if (PTR_ERR(obj) == -ENOMEM) {
 		debug_objects_oom();
 		return;
 	}
-- 
2.50.1.windows.1
Re: [PATCH v3] debugobjects: Fix inconsistent return handling and potential ERR_PTR dereference
Posted by Thomas Gleixner 2 months, 3 weeks ago
On Fri, Nov 14 2025 at 09:56, Haotian Zhang wrote:
> The lookup_object_or_alloc() function can return NULL on memory
> allocation failure, while returning an error pointer for other errors.
> Call sites such as __debug_object_init() and debug_object_activate()
> only check for errors using IS_ERR(), which does not evaluate to true
> for a NULL pointer. This can lead to a NULL pointer dereference if
> memory allocation fails.

Nice fairy tale. Let's look at the facts.

__debug_object_init():
	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
	if (unlikely(!obj)) {
           ....

Does not use IS_ERR() at all and _is_ completely correct because
lookup_object_or_alloc() can only return NULL or a valid object but
never an error pointer because the 'alloc_ifstatic' argument is NULL.

debug_object_activate():
	obj = lookup_object_or_alloc(addr, db, descr, false, true);
	if (unlikely(!obj)) {
           ....
	} else if (likely(!IS_ERR(obj))) {
           ....

handles both the NULL pointer and the error pointer case correctly.

I have no idea which code you were analyzing or which tool halluzinated
about it.

> Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")

There is nothing broken, so this fixes nothing.

Thanks,

        tglx
Re: [PATCH v3] debugobjects: Fix inconsistent return handling and potential ERR_PTR dereference
Posted by Thomas Gleixner 2 months, 3 weeks ago
On Sun, Nov 16 2025 at 00:18, Thomas Gleixner wrote:
> On Fri, Nov 14 2025 at 09:56, Haotian Zhang wrote:
>> The lookup_object_or_alloc() function can return NULL on memory
>> allocation failure, while returning an error pointer for other errors.
>> Call sites such as __debug_object_init() and debug_object_activate()
>> only check for errors using IS_ERR(), which does not evaluate to true
>> for a NULL pointer. This can lead to a NULL pointer dereference if
>> memory allocation fails.
>
> Nice fairy tale. Let's look at the facts.
>
> __debug_object_init():
> 	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
> 	if (unlikely(!obj)) {
>            ....
>
> Does not use IS_ERR() at all and _is_ completely correct because
> lookup_object_or_alloc() can only return NULL or a valid object but
> never an error pointer because the 'alloc_ifstatic' argument is NULL.
>
> debug_object_activate():
> 	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> 	if (unlikely(!obj)) {
>            ....
> 	} else if (likely(!IS_ERR(obj))) {
>            ....
>
> handles both the NULL pointer and the error pointer case correctly.
>
> I have no idea which code you were analyzing or which tool halluzinated
> about it.

That said. You clearly failed to explain how you found that. I'm well
aware that you are deeply involved in LLM based code analysis, so don't
tell me that reviewing random code is your new hobby.

Thanks,

        tglx