diff mbox series

[v6,12/12] rpmsg: Fix kfree() of static memory on setting driver_override

Message ID 20220403183758.192236-13-krzysztof.kozlowski@linaro.org
State Superseded
Headers show
Series Fix broken usage of driver_override (and kfree of static memory) | expand

Commit Message

Krzysztof Kozlowski April 3, 2022, 6:37 p.m. UTC
The driver_override field from platform driver should not be initialized
from static memory (string literal) because the core later kfree() it,
for example when driver_override is set via sysfs.

Use dedicated helper to set driver_override properly.

Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone driver")
Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++--
 drivers/rpmsg/rpmsg_ns.c       | 14 ++++++++++++--
 include/linux/rpmsg.h          |  6 ++++--
 3 files changed, 27 insertions(+), 6 deletions(-)

Comments

Biju Das April 12, 2022, 2:10 p.m. UTC | #1
Hi Krzysztof Kozlowski,

Thanks for the patch.

> Subject: [PATCH v6 12/12] rpmsg: Fix kfree() of static memory on setting
> driver_override
> 
> The driver_override field from platform driver should not be initialized
> from static memory (string literal) because the core later kfree() it, for
> example when driver_override is set via sysfs.
> 
> Use dedicated helper to set driver_override properly.
> 
> Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone driver")
> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++--
>  drivers/rpmsg/rpmsg_ns.c       | 14 ++++++++++++--
>  include/linux/rpmsg.h          |  6 ++++--
>  3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_internal.h
> b/drivers/rpmsg/rpmsg_internal.h index d4b23fd019a8..1a2fb8edf5d3 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -94,10 +94,19 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
>   */
>  static inline int rpmsg_ctrldev_register_device(struct rpmsg_device
> *rpdev)  {
> +	int ret;
> +
>  	strcpy(rpdev->id.name, "rpmsg_ctrl");
> -	rpdev->driver_override = "rpmsg_ctrl";
> +	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
> +				  "rpmsg_ctrl", strlen("rpmsg_ctrl"));

Is it not possible to use rpdev->id.name instead of "rpmsg_ctrl" ?
rpdev->id.name has "rpmsg_ctrl" from strcpy(rpdev->id.name, "rpmsg_ctrl");

Same for "rpmsg_ns" as well

Cheers,
Biju


> +	if (ret)
> +		return ret;
> +
> +	ret = rpmsg_register_device(rpdev);
> +	if (ret)
> +		kfree(rpdev->driver_override);
> 
> -	return rpmsg_register_device(rpdev);
> +	return ret;
>  }
> 
>  #endif
> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c index
> 762ff1ae279f..95a51543f5ad 100644
> --- a/drivers/rpmsg/rpmsg_ns.c
> +++ b/drivers/rpmsg/rpmsg_ns.c
> @@ -20,12 +20,22 @@
>   */
>  int rpmsg_ns_register_device(struct rpmsg_device *rpdev)  {
> +	int ret;
> +
>  	strcpy(rpdev->id.name, "rpmsg_ns");
> -	rpdev->driver_override = "rpmsg_ns";
> +	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
> +				  "rpmsg_ns", strlen("rpmsg_ns"));
> +	if (ret)
> +		return ret;
> +
>  	rpdev->src = RPMSG_NS_ADDR;
>  	rpdev->dst = RPMSG_NS_ADDR;
> 
> -	return rpmsg_register_device(rpdev);
> +	ret = rpmsg_register_device(rpdev);
> +	if (ret)
> +		kfree(rpdev->driver_override);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(rpmsg_ns_register_device);
> 
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h index
> 02fa9116cd60..20c8cd1cde21 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -41,7 +41,9 @@ struct rpmsg_channel_info {
>   * rpmsg_device - device that belong to the rpmsg bus
>   * @dev: the device struct
>   * @id: device id (used to match between rpmsg drivers and devices)
> - * @driver_override: driver name to force a match
> + * @driver_override: driver name to force a match; do not set directly,
> + *                   because core frees it; use driver_set_override() to
> + *                   set or clear it.
>   * @src: local address
>   * @dst: destination address
>   * @ept: the rpmsg endpoint of this channel @@ -51,7 +53,7 @@ struct
> rpmsg_channel_info {  struct rpmsg_device {
>  	struct device dev;
>  	struct rpmsg_device_id id;
> -	char *driver_override;
> +	const char *driver_override;
>  	u32 src;
>  	u32 dst;
>  	struct rpmsg_endpoint *ept;
> --
> 2.32.0
> 
> 
> _______________________________________________
Krzysztof Kozlowski April 13, 2022, 8:40 a.m. UTC | #2
On 12/04/2022 16:10, Biju Das wrote:
> Hi Krzysztof Kozlowski,
> 
> Thanks for the patch.
> 
>> Subject: [PATCH v6 12/12] rpmsg: Fix kfree() of static memory on setting
>> driver_override
>>
>> The driver_override field from platform driver should not be initialized
>> from static memory (string literal) because the core later kfree() it, for
>> example when driver_override is set via sysfs.
>>
>> Use dedicated helper to set driver_override properly.
>>
>> Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone driver")
>> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>>  drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++--
>>  drivers/rpmsg/rpmsg_ns.c       | 14 ++++++++++++--
>>  include/linux/rpmsg.h          |  6 ++++--
>>  3 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_internal.h
>> b/drivers/rpmsg/rpmsg_internal.h index d4b23fd019a8..1a2fb8edf5d3 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -94,10 +94,19 @@ int rpmsg_release_channel(struct rpmsg_device *rpdev,
>>   */
>>  static inline int rpmsg_ctrldev_register_device(struct rpmsg_device
>> *rpdev)  {
>> +	int ret;
>> +
>>  	strcpy(rpdev->id.name, "rpmsg_ctrl");
>> -	rpdev->driver_override = "rpmsg_ctrl";
>> +	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
>> +				  "rpmsg_ctrl", strlen("rpmsg_ctrl"));
> 
> Is it not possible to use rpdev->id.name instead of "rpmsg_ctrl" ?
> rpdev->id.name has "rpmsg_ctrl" from strcpy(rpdev->id.name, "rpmsg_ctrl");
> 
> Same for "rpmsg_ns" as well

It's possible. I kept the pattern of duplicating the string literal
because original code had it, but I don't mind to change it. In the
output assembler that might be additional instruction - need to
dereference the rpdev pointer - but that does not matter much.


Best regards,
Krzysztof
Biju Das April 13, 2022, 8:47 a.m. UTC | #3
Hi Krzysztof Kozlowski,

> Subject: Re: [PATCH v6 12/12] rpmsg: Fix kfree() of static memory on
> setting driver_override
> 
> On 12/04/2022 16:10, Biju Das wrote:
> > Hi Krzysztof Kozlowski,
> >
> > Thanks for the patch.
> >
> >> Subject: [PATCH v6 12/12] rpmsg: Fix kfree() of static memory on
> >> setting driver_override
> >>
> >> The driver_override field from platform driver should not be
> >> initialized from static memory (string literal) because the core
> >> later kfree() it, for example when driver_override is set via sysfs.
> >>
> >> Use dedicated helper to set driver_override properly.
> >>
> >> Fixes: 950a7388f02b ("rpmsg: Turn name service into a stand alone
> >> driver")
> >> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint
> >> interface")
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >> ---
> >>  drivers/rpmsg/rpmsg_internal.h | 13 +++++++++++--
> >>  drivers/rpmsg/rpmsg_ns.c       | 14 ++++++++++++--
> >>  include/linux/rpmsg.h          |  6 ++++--
> >>  3 files changed, 27 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_internal.h
> >> b/drivers/rpmsg/rpmsg_internal.h index d4b23fd019a8..1a2fb8edf5d3
> >> 100644
> >> --- a/drivers/rpmsg/rpmsg_internal.h
> >> +++ b/drivers/rpmsg/rpmsg_internal.h
> >> @@ -94,10 +94,19 @@ int rpmsg_release_channel(struct rpmsg_device
> *rpdev,
> >>   */
> >>  static inline int rpmsg_ctrldev_register_device(struct rpmsg_device
> >> *rpdev)  {
> >> +	int ret;
> >> +
> >>  	strcpy(rpdev->id.name, "rpmsg_ctrl");
> >> -	rpdev->driver_override = "rpmsg_ctrl";
> >> +	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
> >> +				  "rpmsg_ctrl", strlen("rpmsg_ctrl"));
> >
> > Is it not possible to use rpdev->id.name instead of "rpmsg_ctrl" ?
> > rpdev->id.name has "rpmsg_ctrl" from strcpy(rpdev->id.name,
> > rpdev->"rpmsg_ctrl");
> >
> > Same for "rpmsg_ns" as well
> 
> It's possible. I kept the pattern of duplicating the string literal
> because original code had it, but I don't mind to change it. In the output
> assembler that might be additional instruction - need to dereference the
> rpdev pointer - but that does not matter much.
> 

OK, it's a suggestion as same string constant duplicated thrice,

Any change in this string constant in future will need changes
in 3 places , by using "rpdev->id.name", the change is limited
to 1 place.

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index d4b23fd019a8..1a2fb8edf5d3 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -94,10 +94,19 @@  int rpmsg_release_channel(struct rpmsg_device *rpdev,
  */
 static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
 {
+	int ret;
+
 	strcpy(rpdev->id.name, "rpmsg_ctrl");
-	rpdev->driver_override = "rpmsg_ctrl";
+	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
+				  "rpmsg_ctrl", strlen("rpmsg_ctrl"));
+	if (ret)
+		return ret;
+
+	ret = rpmsg_register_device(rpdev);
+	if (ret)
+		kfree(rpdev->driver_override);
 
-	return rpmsg_register_device(rpdev);
+	return ret;
 }
 
 #endif
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c
index 762ff1ae279f..95a51543f5ad 100644
--- a/drivers/rpmsg/rpmsg_ns.c
+++ b/drivers/rpmsg/rpmsg_ns.c
@@ -20,12 +20,22 @@ 
  */
 int rpmsg_ns_register_device(struct rpmsg_device *rpdev)
 {
+	int ret;
+
 	strcpy(rpdev->id.name, "rpmsg_ns");
-	rpdev->driver_override = "rpmsg_ns";
+	ret = driver_set_override(&rpdev->dev, &rpdev->driver_override,
+				  "rpmsg_ns", strlen("rpmsg_ns"));
+	if (ret)
+		return ret;
+
 	rpdev->src = RPMSG_NS_ADDR;
 	rpdev->dst = RPMSG_NS_ADDR;
 
-	return rpmsg_register_device(rpdev);
+	ret = rpmsg_register_device(rpdev);
+	if (ret)
+		kfree(rpdev->driver_override);
+
+	return ret;
 }
 EXPORT_SYMBOL(rpmsg_ns_register_device);
 
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 02fa9116cd60..20c8cd1cde21 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -41,7 +41,9 @@  struct rpmsg_channel_info {
  * rpmsg_device - device that belong to the rpmsg bus
  * @dev: the device struct
  * @id: device id (used to match between rpmsg drivers and devices)
- * @driver_override: driver name to force a match
+ * @driver_override: driver name to force a match; do not set directly,
+ *                   because core frees it; use driver_set_override() to
+ *                   set or clear it.
  * @src: local address
  * @dst: destination address
  * @ept: the rpmsg endpoint of this channel
@@ -51,7 +53,7 @@  struct rpmsg_channel_info {
 struct rpmsg_device {
 	struct device dev;
 	struct rpmsg_device_id id;
-	char *driver_override;
+	const char *driver_override;
 	u32 src;
 	u32 dst;
 	struct rpmsg_endpoint *ept;