linux-kernel - Re: [PATCH v4 1/3] rpmsg: char: Fix WARN() in error path of rpmsg_eptdev

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9494f76b-0cd1-42be-9f11-2a5b82e76e6d@oss.qualcomm.com>
Date: Wed, 19 Nov 2025 20:26:42 +0800
From: Zhongqiu Han <zhongqiu.han@....qualcomm.com>
To: Dawei Li <dawei.li@...ux.dev>
Cc: andersson@...nel.org, mathieu.poirier@...aro.org,
        linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        set_pte_at@...look.com, zhongqiu.han@....qualcomm.com
Subject: Re: [PATCH v4 1/3] rpmsg: char: Fix WARN() in error path of
 rpmsg_eptdev_add()

On 11/19/2025 6:56 PM, Dawei Li wrote:
> On Wed, Nov 19, 2025 at 12:07:02PM +0800, Zhongqiu Han wrote:
>> On 11/18/2025 11:41 PM, Dawei Li wrote:
>>> put_device() is called on error path of rpmsg_eptdev_add() to cleanup
>>> resource attached to eptdev->dev, unfortunately it's bogus cause
>>> dev->release() is not set yet.
>>>
>>> When a struct device instance is destroyed, driver core framework checks
>>> the possible release() callback from candidates below:
>>> - struct device::release()
>>> - dev->type->release()
>>> - dev->class->dev_release()
>>>
>>> Rpmsg eptdev owns none of them so WARN() will complain the absence of
>>> release().
>>>
>>> Fix it by:
>>> - Pre-assign dev->release() before potential error path.
>>> - Check before ida_free() in dev->release().
>>>
>>> By fixing error path of rpmsg_eptdev_add() and fixing potential memory
>>> leak in rpmsg_anonymous_eptdev_create(), this work paves the way of rework
>>> of rpmsg_eptdev_add() and its callers.
>>>
>>> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
>>> Signed-off-by: Dawei Li <dawei.li@...ux.dev>
>>> ---
>>>    drivers/rpmsg/rpmsg_char.c | 26 +++++++++++++-------------
>>>    1 file changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>> index 34b35ea74aab..373b627581e8 100644
>>> --- a/drivers/rpmsg/rpmsg_char.c
>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>> @@ -408,8 +408,13 @@ static void rpmsg_eptdev_release_device(struct device *dev)
>>>    {
>>>    	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>>> -	ida_free(&rpmsg_ept_ida, dev->id);
>>> -	if (eptdev->dev.devt)
>>> +	/*
>>> +	 * release() can be invoked from error path of rpmsg_eptdev_add(),
>>> +	 * WARN() will be fired if ida_free() is feed with invalid ID.
>>> +	 */
>>> +	if (likely(ida_exists(&rpmsg_ept_ida, dev->id)))
>>> +		ida_free(&rpmsg_ept_ida, dev->id);
>>> +	if (eptdev->dev.devt && likely(ida_exists(&rpmsg_minor_ida, MINOR(eptdev->dev.devt))))
>>>    		ida_free(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
>>>    	kfree(eptdev);
>>>    }
>>> @@ -458,6 +463,8 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
>>>    	struct device *dev = &eptdev->dev;
>>>    	int ret;
>>> +	dev->release = rpmsg_eptdev_release_device;
>>> +
>>>    	eptdev->chinfo = chinfo;
>>>    	if (cdev) {
>>> @@ -471,7 +478,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
>>>    	/* Anonymous inode device still need device name for dev_err() and friends */
>>>    	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
>>>    	if (ret < 0)
>>> -		goto free_minor_ida;
>>> +		goto free_eptdev;
>>>    	dev->id = ret;
>>>    	dev_set_name(dev, "rpmsg%d", ret);
>>> @@ -480,22 +487,13 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
>>>    	if (cdev) {
>>>    		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
>>>    		if (ret)
>>> -			goto free_ept_ida;
>>> +			goto free_eptdev;
>>>    	}
>>> -	/* We can now rely on the release function for cleanup */
>>> -	dev->release = rpmsg_eptdev_release_device;
>>> -
>>>    	return ret;
>>> -free_ept_ida:
>>> -	ida_free(&rpmsg_ept_ida, dev->id);
>>> -free_minor_ida:
>>> -	if (cdev)
>>> -		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
>>>    free_eptdev:
>>>    	put_device(dev);
>>> -	kfree(eptdev);
>>
>>
>> Hi Dawei,
>>
>> Thanks for your new version~
>>
>> Patch 1/3 will introduce a use-after-free of eptdev in func
>> rpmsg_anonymous_eptdev_create(),
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c?h=for-next#n548
>>
>> even though this issue will be resolved in 2/3. However, 1/3, as an
>> independent commit, should not introduce a new bug.
> 
> FWIW, it's not new bug introduced by this commit, it's introduced by
> 2410558f5f11, which is supposed to be fixed in patch[2/3].

Hi Dawei,
I checked commit 2410558f5f11 and see that the issue existed before this
patch. Thanks for clarifying,


> 
> And new reorganize of series is trying to address the comments from
> Mathieu[1], If I understand it correctly.
> 
> Bjorn, Mathieu,
> What's your inputs on this? I will respin v5 if you find it necessary.
> 
> Diff between v5 from v4:
> patch[1/3]: Remove dev_err(&eptdev->dev) from rpmsg_anonymous_eptdev_create();
> 
> patch[2/3]: Bring back dev_err(&eptdev->dev) to rpmsg_anonymous_eptdev_create();
> 
> [1] https://lore.kernel.org/all/aRd8mDzQWXtEFnmt@p14s/
> 
>>
>>
>>>    	return ret;
>>>    }
>>> @@ -561,6 +559,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
>>>    	if (!ret)
>>>    		*pfd = fd;
>>> +	else
>>> +		put_device(&eptdev->dev);
>>>    	return ret;
>>>    }
>>
>>
>> -- 
>> Thx and BRs,
>> Zhongqiu Han
> 
> Thanks,
> 
> 	Dawei


-- 
Thx and BRs,
Zhongqiu Han