* [PATCH 1/3] cpufreq: qcom: fix memory leak in error path
@ 2022-10-01 17:10 Fabien Parent
2022-10-01 17:10 ` [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer Fabien Parent
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Fabien Parent @ 2022-10-01 17:10 UTC (permalink / raw)
To: ilia.lin, agross, bjorn.andersson, rafael, viresh.kumar
Cc: linux-pm, linux-arm-msm, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer 2022-10-01 17:10 [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Fabien Parent @ 2022-10-01 17:10 ` Fabien Parent 2022-10-10 5:52 ` Viresh Kumar 2022-10-01 17:10 ` [PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region Fabien Parent 2022-10-10 5:55 ` [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Viresh Kumar 2 siblings, 1 reply; 9+ messages in thread From: Fabien Parent @ 2022-10-01 17:10 UTC (permalink / raw) To: ilia.lin, agross, bjorn.andersson, rafael, viresh.kumar Cc: linux-pm, linux-arm-msm, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer 2022-10-01 17:10 ` [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer Fabien Parent @ 2022-10-10 5:52 ` Viresh Kumar 2022-10-10 6:01 ` Viresh Kumar 0 siblings, 1 reply; 9+ messages in thread From: Viresh Kumar @ 2022-10-10 5:52 UTC (permalink / raw) To: Fabien Parent Cc: ilia.lin, agross, bjorn.andersson, rafael, linux-pm, linux-arm-msm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer 2022-10-10 5:52 ` Viresh Kumar @ 2022-10-10 6:01 ` Viresh Kumar 0 siblings, 0 replies; 9+ messages in thread From: Viresh Kumar @ 2022-10-10 6:01 UTC (permalink / raw) To: Fabien Parent Cc: ilia.lin, agross, bjorn.andersson, rafael, linux-pm, linux-arm-msm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region 2022-10-01 17:10 [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Fabien Parent 2022-10-01 17:10 ` [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer Fabien Parent @ 2022-10-01 17:10 ` Fabien Parent 2022-10-10 6:00 ` Viresh Kumar 2022-10-10 5:55 ` [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Viresh Kumar 2 siblings, 1 reply; 9+ messages in thread From: Fabien Parent @ 2022-10-01 17:10 UTC (permalink / raw) To: ilia.lin, agross, bjorn.andersson, rafael, viresh.kumar Cc: linux-pm, linux-arm-msm, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region 2022-10-01 17:10 ` [PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region Fabien Parent @ 2022-10-10 6:00 ` Viresh Kumar 0 siblings, 0 replies; 9+ messages in thread From: Viresh Kumar @ 2022-10-10 6:00 UTC (permalink / raw) To: Fabien Parent Cc: ilia.lin, agross, bjorn.andersson, rafael, linux-pm, linux-arm-msm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cpufreq: qcom: fix memory leak in error path 2022-10-01 17:10 [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Fabien Parent 2022-10-01 17:10 ` [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer Fabien Parent 2022-10-01 17:10 ` [PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region Fabien Parent @ 2022-10-10 5:55 ` Viresh Kumar 2022-10-10 6:02 ` Viresh Kumar 2 siblings, 1 reply; 9+ messages in thread From: Viresh Kumar @ 2022-10-10 5:55 UTC (permalink / raw) To: Fabien Parent Cc: ilia.lin, agross, bjorn.andersson, rafael, linux-pm, linux-arm-msm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cpufreq: qcom: fix memory leak in error path 2022-10-10 5:55 ` [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Viresh Kumar @ 2022-10-10 6:02 ` Viresh Kumar 2022-10-15 12:57 ` Fabien Parent 0 siblings, 1 reply; 9+ messages in thread From: Viresh Kumar @ 2022-10-10 6:02 UTC (permalink / raw) To: Fabien Parent Cc: ilia.lin, agross, bjorn.andersson, rafael, linux-pm, linux-arm-msm, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cpufreq: qcom: fix memory leak in error path 2022-10-10 6:02 ` Viresh Kumar @ 2022-10-15 12:57 ` Fabien Parent 0 siblings, 0 replies; 9+ messages in thread From: Fabien Parent @ 2022-10-15 12:57 UTC (permalink / raw) To: Viresh Kumar Cc: ilia.lin, agross, bjorn.andersson, rafael, linux-pm, linux-arm-msm, linux-kernel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-10-15 12:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-01 17:10 [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Fabien Parent 2022-10-01 17:10 ` [PATCH 2/3] cpufreq: qcom: pass pvs_name size along with its buffer Fabien Parent 2022-10-10 5:52 ` Viresh Kumar 2022-10-10 6:01 ` Viresh Kumar 2022-10-01 17:10 ` [PATCH 3/3] cpufreq: qcom: fix writes in read-only memory region Fabien Parent 2022-10-10 6:00 ` Viresh Kumar 2022-10-10 5:55 ` [PATCH 1/3] cpufreq: qcom: fix memory leak in error path Viresh Kumar 2022-10-10 6:02 ` Viresh Kumar 2022-10-15 12:57 ` Fabien Parent
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).