[PATCH 1/3] cpufreq: qcom: fix memory leak in error path

[PATCH 1/3] cpufreq: qcom: fix memory leak in error path

[Fabien Parent]

If for some reason the speedbin length is incorrect, then there is a
memory leak in the error path because we never free the speedbin buffer.
This commit fixes the error path to always free the speedbin buffer.

Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-nvmem.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index 863548f59c3e..3bd38acde4b9 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -213,6 +213,7 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
 	int speed = 0, pvs = 0, pvs_ver = 0;
 	u8 *speedbin;
 	size_t len;
+	int ret = 0;
 
 	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
 
@@ -230,7 +231,8 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
 		break;
 	default:
 		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto len_error;
 	}
 
 	snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
@@ -238,8 +240,9 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
 
 	drv->versions = (1 << speed);
 
+len_error:
 	kfree(speedbin);
-	return 0;
+	return ret;
 }
 
 static const struct qcom_cpufreq_match_data match_data_kryo = {
-- 
2.37.2

[PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer

[Fabien Parent]

The get_version handler takes a pvs_name buffer and can override it with
the speed, pvs, and pvs version values. The function does not take as
argument the buffer size and is currently being determined by calling
`sizeof("speedXX-pvsXX-vXX")`. This is not great because it duplicates
the string in several locations which makes it error-prone if we need to
modify the string someday. Also since the buffer and its size are tied
together, it makes sense that they should both be passed together to the
get_version as parameters.

This commit makes sure that the PVS name template string is only
defined once, and that the pvs_name buffer is passed with its size.

Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-nvmem.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index 3bd38acde4b9..64ce077a4848 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -30,6 +30,7 @@
 #include <linux/soc/qcom/smem.h>
 
 #define MSM_ID_SMEM	137
+#define PVS_NAME_TEMPLATE "speedXX-pvsXX-vXX"
 
 enum _msm_id {
 	MSM8996V3 = 0xF6ul,
@@ -50,6 +51,7 @@ struct qcom_cpufreq_match_data {
 	int (*get_version)(struct device *cpu_dev,
 			   struct nvmem_cell *speedbin_nvmem,
 			   char **pvs_name,
+			   size_t pvs_name_size,
 			   struct qcom_cpufreq_drv *drv);
 	const char **genpd_names;
 };
@@ -172,6 +174,7 @@ static enum _msm8996_version qcom_cpufreq_get_msm_id(void)
 static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
 					  struct nvmem_cell *speedbin_nvmem,
 					  char **pvs_name,
+					  size_t pvs_name_size,
 					  struct qcom_cpufreq_drv *drv)
 {
 	size_t len;
@@ -208,6 +211,7 @@ static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
 static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
 					   struct nvmem_cell *speedbin_nvmem,
 					   char **pvs_name,
+					   size_t pvs_name_size,
 					   struct qcom_cpufreq_drv *drv)
 {
 	int speed = 0, pvs = 0, pvs_ver = 0;
@@ -235,7 +239,7 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
 		goto len_error;
 	}
 
-	snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
+	snprintf(*pvs_name, pvs_name_size, "speed%d-pvs%d-v%d",
 		 speed, pvs, pvs_ver);
 
 	drv->versions = (1 << speed);
@@ -265,7 +269,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
 	struct nvmem_cell *speedbin_nvmem;
 	struct device_node *np;
 	struct device *cpu_dev;
-	char *pvs_name = "speedXX-pvsXX-vXX";
+	char *pvs_name = PVS_NAME_TEMPLATE;
 	unsigned cpu;
 	const struct of_device_id *match;
 	int ret;
@@ -306,8 +310,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
 			goto free_drv;
 		}
 
-		ret = drv->data->get_version(cpu_dev,
-							speedbin_nvmem, &pvs_name, drv);
+		ret = drv->data->get_version(cpu_dev, speedbin_nvmem, &pvs_name,
+					     sizeof(PVS_NAME_TEMPLATE), drv);
 		if (ret) {
 			nvmem_cell_put(speedbin_nvmem);
 			goto free_drv;
-- 
2.37.2

[PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region

[Fabien Parent]

This commit fixes a kernel oops because of a write in some read-only memory:

	[    9.068287] Unable to handle kernel write to read-only memory at virtual address ffff800009240ad8
	..snip..
	[    9.138790] Internal error: Oops: 9600004f [#1] PREEMPT SMP
	..snip..
	[    9.269161] Call trace:
	[    9.276271]  __memcpy+0x5c/0x230
	[    9.278531]  snprintf+0x58/0x80
	[    9.282002]  qcom_cpufreq_msm8939_name_version+0xb4/0x190
	[    9.284869]  qcom_cpufreq_probe+0xc8/0x39c
	..snip..

The following line defines a pointer that point to a char buffer stored
in read-only memory:

	char *pvs_name = PVS_NAME;

This pointer is meant to hold a template "speedXX-pvsXX-vXX" where the
XX values get overridden by the qcom_cpufreq_krait_name_version function. Since
the template is actually stored in read-only memory, when the function
executes the following call we get an oops:

	snprintf(*pvs_name, sizeof(PVS_NAME), "speed%d-pvs%d-v%d",
		 speed, pvs, pvs_ver);

To fix this issue, we instead store the template name onto the stack by
using the following syntax:

	char pvs_name_buffer[] = PVS_NAME;

Because the `pvs_name` needs to be able to be assigned to NULL, the
template buffer is stored in the pvs_name_template and not under the
pvs_name variable.

Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-nvmem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index 64ce077a4848..3e097262f612 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -269,7 +269,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
 	struct nvmem_cell *speedbin_nvmem;
 	struct device_node *np;
 	struct device *cpu_dev;
-	char *pvs_name = PVS_NAME_TEMPLATE;
+	char pvs_name_buffer[] = PVS_NAME_TEMPLATE;
+	char *pvs_name = pvs_name_buffer;
 	unsigned cpu;
 	const struct of_device_id *match;
 	int ret;
-- 
2.37.2

Re: [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer

[Viresh Kumar]

On 01-10-22, 19:10, Fabien Parent wrote:
> @@ -265,7 +269,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>  	struct nvmem_cell *speedbin_nvmem;
>  	struct device_node *np;
>  	struct device *cpu_dev;
> -	char *pvs_name = "speedXX-pvsXX-vXX";
> +	char *pvs_name = PVS_NAME_TEMPLATE;
>  	unsigned cpu;
>  	const struct of_device_id *match;
>  	int ret;
> @@ -306,8 +310,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>  			goto free_drv;
>  		}
>  
> -		ret = drv->data->get_version(cpu_dev,
> -							speedbin_nvmem, &pvs_name, drv);
> +		ret = drv->data->get_version(cpu_dev, speedbin_nvmem, &pvs_name,
> +					     sizeof(PVS_NAME_TEMPLATE), drv);

Since the pvs name is always PVS_NAME_TEMPLATE, why are we even
passing it and size here ? Why not directly use it in those
get_version() implementations directly ?

>  		if (ret) {
>  			nvmem_cell_put(speedbin_nvmem);
>  			goto free_drv;
> -- 
> 2.37.2

-- 
viresh

Re: [PATCH 1/3] cpufreq: qcom: fix memory leak in error path

[Viresh Kumar]

On 01-10-22, 19:10, Fabien Parent wrote:
> If for some reason the speedbin length is incorrect, then there is a
> memory leak in the error path because we never free the speedbin buffer.
> This commit fixes the error path to always free the speedbin buffer.
> 
> Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-nvmem.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index 863548f59c3e..3bd38acde4b9 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -213,6 +213,7 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
>  	int speed = 0, pvs = 0, pvs_ver = 0;
>  	u8 *speedbin;
>  	size_t len;
> +	int ret = 0;
>  
>  	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
>  
> @@ -230,7 +231,8 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
>  		break;
>  	default:
>  		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto len_error;
>  	}
>  
>  	snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> @@ -238,8 +240,9 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
>  
>  	drv->versions = (1 << speed);
>  
> +len_error:
>  	kfree(speedbin);
> -	return 0;
> +	return ret;
>  }
>  
>  static const struct qcom_cpufreq_match_data match_data_kryo = {

Applied. Thanks.

-- 
viresh

Re: [PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region

[Viresh Kumar]

On 01-10-22, 19:10, Fabien Parent wrote:
> This commit fixes a kernel oops because of a write in some read-only memory:
> 
> 	[    9.068287] Unable to handle kernel write to read-only memory at virtual address ffff800009240ad8
> 	..snip..
> 	[    9.138790] Internal error: Oops: 9600004f [#1] PREEMPT SMP
> 	..snip..
> 	[    9.269161] Call trace:
> 	[    9.276271]  __memcpy+0x5c/0x230
> 	[    9.278531]  snprintf+0x58/0x80
> 	[    9.282002]  qcom_cpufreq_msm8939_name_version+0xb4/0x190
> 	[    9.284869]  qcom_cpufreq_probe+0xc8/0x39c
> 	..snip..
> 
> The following line defines a pointer that point to a char buffer stored
> in read-only memory:
> 
> 	char *pvs_name = PVS_NAME;
> 
> This pointer is meant to hold a template "speedXX-pvsXX-vXX" where the
> XX values get overridden by the qcom_cpufreq_krait_name_version function. Since
> the template is actually stored in read-only memory, when the function
> executes the following call we get an oops:
> 
> 	snprintf(*pvs_name, sizeof(PVS_NAME), "speed%d-pvs%d-v%d",
> 		 speed, pvs, pvs_ver);
> 
> To fix this issue, we instead store the template name onto the stack by
> using the following syntax:
> 
> 	char pvs_name_buffer[] = PVS_NAME;
> 
> Because the `pvs_name` needs to be able to be assigned to NULL, the
> template buffer is stored in the pvs_name_template and not under the
> pvs_name variable.
> 
> Signed-off-by: Fabien Parent <fabien.parent@linaro.org>

No Fixes or Cc:Stable for this ? This looks like a bug fix and this
should be sent before patch 2/3 so it can be back ported properly.

> ---
>  drivers/cpufreq/qcom-cpufreq-nvmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index 64ce077a4848..3e097262f612 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -269,7 +269,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>  	struct nvmem_cell *speedbin_nvmem;
>  	struct device_node *np;
>  	struct device *cpu_dev;
> -	char *pvs_name = PVS_NAME_TEMPLATE;
> +	char pvs_name_buffer[] = PVS_NAME_TEMPLATE;
> +	char *pvs_name = pvs_name_buffer;
>  	unsigned cpu;
>  	const struct of_device_id *match;
>  	int ret;
> -- 
> 2.37.2

-- 
viresh

Re: [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer

[Viresh Kumar]

On 10-10-22, 11:22, Viresh Kumar wrote:
> On 01-10-22, 19:10, Fabien Parent wrote:
> > @@ -265,7 +269,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> >  	struct nvmem_cell *speedbin_nvmem;
> >  	struct device_node *np;
> >  	struct device *cpu_dev;
> > -	char *pvs_name = "speedXX-pvsXX-vXX";
> > +	char *pvs_name = PVS_NAME_TEMPLATE;
> >  	unsigned cpu;
> >  	const struct of_device_id *match;
> >  	int ret;
> > @@ -306,8 +310,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> >  			goto free_drv;
> >  		}
> >  
> > -		ret = drv->data->get_version(cpu_dev,
> > -							speedbin_nvmem, &pvs_name, drv);
> > +		ret = drv->data->get_version(cpu_dev, speedbin_nvmem, &pvs_name,
> > +					     sizeof(PVS_NAME_TEMPLATE), drv);
> 
> Since the pvs name is always PVS_NAME_TEMPLATE, why are we even
> passing it and size here ? Why not directly use it in those
> get_version() implementations directly ?

I understand how &pvs_name is used here to later set the prop_name. I
think instead you should also implement drv->data->prop_name() here,
which can return valid name or NULL.

-- 
viresh

Re: [PATCH 1/3] cpufreq: qcom: fix memory leak in error path

[Viresh Kumar]

On 10-10-22, 11:25, Viresh Kumar wrote:
> On 01-10-22, 19:10, Fabien Parent wrote:
> > If for some reason the speedbin length is incorrect, then there is a
> > memory leak in the error path because we never free the speedbin buffer.
> > This commit fixes the error path to always free the speedbin buffer.
> > 
> > Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> > ---
> >  drivers/cpufreq/qcom-cpufreq-nvmem.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > index 863548f59c3e..3bd38acde4b9 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > @@ -213,6 +213,7 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
> >  	int speed = 0, pvs = 0, pvs_ver = 0;
> >  	u8 *speedbin;
> >  	size_t len;
> > +	int ret = 0;
> >  
> >  	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> >  
> > @@ -230,7 +231,8 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
> >  		break;
> >  	default:
> >  		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> > +		goto len_error;
> >  	}
> >  
> >  	snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> > @@ -238,8 +240,9 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
> >  
> >  	drv->versions = (1 << speed);
> >  
> > +len_error:
> >  	kfree(speedbin);
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static const struct qcom_cpufreq_match_data match_data_kryo = {
> 
> Applied. Thanks.

Btw, it will be good to have a Fixes or Cc:Stable for this patch too.
I can directly add the lines myself, just let me know what you want.

-- 
viresh

Re: [PATCH 1/3] cpufreq: qcom: fix memory leak in error path

[Fabien Parent]

Hi Viresh,

> > Applied. Thanks.
>
> Btw, it will be good to have a Fixes or Cc:Stable for this patch too.
> I can directly add the lines myself, just let me know what you want.

I will send the patch with the "Fixes" tag in v2.