linux-kernel - Re: [PATCH] rpmsg: core: fix race in driver_override

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] [day] [month] [year] [list]
Message-ID: <e11fa440-a278-4e39-adb5-244eb04ec702@oss.qualcomm.com>
Date: Wed, 3 Dec 2025 17:00:46 +0800
From: Zhongqiu Han <zhongqiu.han@....qualcomm.com>
To: Gui-Dong Han <hanguidong02@...il.com>, andersson@...nel.org,
        mathieu.poirier@...aro.org
Cc: linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, zhongqiu.han@....qualcomm.com
Subject: Re: [PATCH] rpmsg: core: fix race in driver_override_show() and use
 core helper

On 12/3/2025 1:49 AM, Gui-Dong Han wrote:
> The driver_override_show function reads the driver_override string
> without holding the device_lock. However, the store function modifies
> and frees the string while holding the device_lock. This creates a race
> condition where the string can be freed by the store function while
> being read by the show function, leading to a use-after-free.
> 
> To fix this, replace the rpmsg_string_attr macro with explicit show and
> store functions. The new driver_override_store uses the standard
> driver_set_override helper. Since the introduction of
> driver_set_override, the comments in include/linux/rpmsg.h have stated
> that this helper must be used to set or clear driver_override, but the
> implementation was not updated until now.
> 
> Because driver_set_override modifies and frees the string while holding
> the device_lock, the new driver_override_show now correctly holds the
> device_lock during the read operation to prevent the race.
> 
> Additionally, since rpmsg_string_attr has only ever been used for
> driver_override, removing the macro simplifies the code.
> 
> Fixes: 39e47767ec9b ("rpmsg: Add driver_override device attribute for rpmsg_device")
> Cc: stable@...r.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@...il.com>

Reviewed-by: Zhongqiu Han <zhongqiu.han@....qualcomm.com>

> ---
> I verified this with a stress test that continuously writes/reads the
> attribute. It triggered KASAN and leaked bytes like a0 f4 81 9f a3 ff ff
> (likely kernel pointers). Since driver_override is world-readable (0644),
> this allows unprivileged users to leak kernel pointers and bypass KASLR.
> Similar races were fixed in other buses (e.g., commits 9561475db680 and
> 91d44c1afc61). Currently, 9 of 11 buses handle this correctly; this patch
> fixes one of the remaining two.
> ---
>   drivers/rpmsg/rpmsg_core.c | 66 ++++++++++++++++----------------------
>   1 file changed, 27 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 5d661681a9b6..96964745065b 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -352,50 +352,38 @@ field##_show(struct device *dev,					\
>   }									\
>   static DEVICE_ATTR_RO(field);
>   
> -#define rpmsg_string_attr(field, member)				\
> -static ssize_t								\
> -field##_store(struct device *dev, struct device_attribute *attr,	\
> -	      const char *buf, size_t sz)				\
> -{									\
> -	struct rpmsg_device *rpdev = to_rpmsg_device(dev);		\
> -	const char *old;						\
> -	char *new;							\
> -									\
> -	new = kstrndup(buf, sz, GFP_KERNEL);				\
> -	if (!new)							\
> -		return -ENOMEM;						\
> -	new[strcspn(new, "\n")] = '\0';					\
> -									\
> -	device_lock(dev);						\
> -	old = rpdev->member;						\
> -	if (strlen(new)) {						\
> -		rpdev->member = new;					\
> -	} else {							\
> -		kfree(new);						\
> -		rpdev->member = NULL;					\
> -	}								\
> -	device_unlock(dev);						\
> -									\
> -	kfree(old);							\
> -									\
> -	return sz;							\
> -}									\
> -static ssize_t								\
> -field##_show(struct device *dev,					\
> -	     struct device_attribute *attr, char *buf)			\
> -{									\
> -	struct rpmsg_device *rpdev = to_rpmsg_device(dev);		\
> -									\
> -	return sprintf(buf, "%s\n", rpdev->member);			\
> -}									\
> -static DEVICE_ATTR_RW(field)
> -
>   /* for more info, see Documentation/ABI/testing/sysfs-bus-rpmsg */
>   rpmsg_show_attr(name, id.name, "%s\n");
>   rpmsg_show_attr(src, src, "0x%x\n");
>   rpmsg_show_attr(dst, dst, "0x%x\n");
>   rpmsg_show_attr(announce, announce ? "true" : "false", "%s\n");
> -rpmsg_string_attr(driver_override, driver_override);
> +
> +static ssize_t driver_override_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> +	int ret;
> +
> +	ret = driver_set_override(dev, &rpdev->driver_override, buf, count);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t driver_override_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> +	ssize_t len;
> +
> +	device_lock(dev);
> +	len = sysfs_emit(buf, "%s\n", rpdev->driver_override);
> +	device_unlock(dev);
> +	return len;
> +}
> +static DEVICE_ATTR_RW(driver_override);
>   
>   static ssize_t modalias_show(struct device *dev,
>   			     struct device_attribute *attr, char *buf)


-- 
Thx and BRs,
Zhongqiu Han