* [PATCH][next] clk: vc5: fix use of memory after it has been kfree'd
@ 2020-06-25 13:27 Colin King
2020-07-21 10:10 ` Luca Ceresoli
2020-07-23 1:46 ` Stephen Boyd
0 siblings, 2 replies; 3+ messages in thread
From: Colin King @ 2020-06-25 13:27 UTC (permalink / raw)
To: Marek Vasut, Michael Turquette, Stephen Boyd, Adam Ford, linux-clk
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There are a several places where printing an error message of
init.name occurs after init.name has been kfree'd. Also the failure
message is duplicated each time in the code. Fix this by adding
a registration error failure path for these cases, moving the
duplicated error messages to one common point and kfree'ing init.name
only after it has been used.
Changes also shrink the object code size by 171 bytes (x86-64, gcc 9.3):
Before:
text data bss dec hex filename
21057 3960 64 25081 61f9 drivers/clk/clk-versaclock5.o
After:
text data bss dec hex filename
20886 3960 64 24910 614e drivers/clk/clk-versaclock5.o
Addresses-Coverity: ("Use after free")
Fixes: f491276a5168 ("clk: vc5: Allow Versaclock driver to support multiple instances")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/clk/clk-versaclock5.c | 51 +++++++++++++----------------------
1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index 9a5fb3834b9a..1d8ee4b8b1f5 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -882,11 +882,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
init.parent_names = parent_names;
vc5->clk_mux.init = &init;
ret = devm_clk_hw_register(&client->dev, &vc5->clk_mux);
+ if (ret)
+ goto err_clk_register;
kfree(init.name); /* clock framework made a copy of the name */
- if (ret) {
- dev_err(&client->dev, "unable to register %s\n", init.name);
- goto err_clk;
- }
if (vc5->chip_info->flags & VC5_HAS_PFD_FREQ_DBL) {
/* Register frequency doubler */
@@ -900,12 +898,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
init.num_parents = 1;
vc5->clk_mul.init = &init;
ret = devm_clk_hw_register(&client->dev, &vc5->clk_mul);
+ if (ret)
+ goto err_clk_register;
kfree(init.name); /* clock framework made a copy of the name */
- if (ret) {
- dev_err(&client->dev, "unable to register %s\n",
- init.name);
- goto err_clk;
- }
}
/* Register PFD */
@@ -921,11 +916,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
init.num_parents = 1;
vc5->clk_pfd.init = &init;
ret = devm_clk_hw_register(&client->dev, &vc5->clk_pfd);
+ if (ret)
+ goto err_clk_register;
kfree(init.name); /* clock framework made a copy of the name */
- if (ret) {
- dev_err(&client->dev, "unable to register %s\n", init.name);
- goto err_clk;
- }
/* Register PLL */
memset(&init, 0, sizeof(init));
@@ -939,11 +932,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
vc5->clk_pll.vc5 = vc5;
vc5->clk_pll.hw.init = &init;
ret = devm_clk_hw_register(&client->dev, &vc5->clk_pll.hw);
+ if (ret)
+ goto err_clk_register;
kfree(init.name); /* clock framework made a copy of the name */
- if (ret) {
- dev_err(&client->dev, "unable to register %s\n", init.name);
- goto err_clk;
- }
/* Register FODs */
for (n = 0; n < vc5->chip_info->clk_fod_cnt; n++) {
@@ -960,12 +951,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
vc5->clk_fod[n].vc5 = vc5;
vc5->clk_fod[n].hw.init = &init;
ret = devm_clk_hw_register(&client->dev, &vc5->clk_fod[n].hw);
+ if (ret)
+ goto err_clk_register;
kfree(init.name); /* clock framework made a copy of the name */
- if (ret) {
- dev_err(&client->dev, "unable to register %s\n",
- init.name);
- goto err_clk;
- }
}
/* Register MUX-connected OUT0_I2C_SELB output */
@@ -981,11 +969,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
vc5->clk_out[0].vc5 = vc5;
vc5->clk_out[0].hw.init = &init;
ret = devm_clk_hw_register(&client->dev, &vc5->clk_out[0].hw);
- kfree(init.name); /* clock framework made a copy of the name */
- if (ret) {
- dev_err(&client->dev, "unable to register %s\n", init.name);
- goto err_clk;
- }
+ if (ret)
+ goto err_clk_register;
+ kfree(init.name); /* clock framework made a copy of the name */
/* Register FOD-connected OUTx outputs */
for (n = 1; n < vc5->chip_info->clk_out_cnt; n++) {
@@ -1008,17 +994,15 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
vc5->clk_out[n].vc5 = vc5;
vc5->clk_out[n].hw.init = &init;
ret = devm_clk_hw_register(&client->dev, &vc5->clk_out[n].hw);
+ if (ret)
+ goto err_clk_register;
kfree(init.name); /* clock framework made a copy of the name */
- if (ret) {
- dev_err(&client->dev, "unable to register %s\n",
- init.name);
- goto err_clk;
- }
/* Fetch Clock Output configuration from DT (if specified) */
ret = vc5_get_output_config(client, &vc5->clk_out[n]);
if (ret)
goto err_clk;
+
}
ret = of_clk_add_hw_provider(client->dev.of_node, vc5_of_clk_get, vc5);
@@ -1029,6 +1013,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
return 0;
+err_clk_register:
+ dev_err(&client->dev, "unable to register %s\n", init.name);
+ kfree(init.name); /* clock framework made a copy of the name */
err_clk:
if (vc5->chip_info->flags & VC5_HAS_INTERNAL_XTAL)
clk_unregister_fixed_rate(vc5->pin_xin);
--
2.27.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH][next] clk: vc5: fix use of memory after it has been kfree'd
2020-06-25 13:27 [PATCH][next] clk: vc5: fix use of memory after it has been kfree'd Colin King
@ 2020-07-21 10:10 ` Luca Ceresoli
2020-07-23 1:46 ` Stephen Boyd
1 sibling, 0 replies; 3+ messages in thread
From: Luca Ceresoli @ 2020-07-21 10:10 UTC (permalink / raw)
To: Colin King, Marek Vasut, Michael Turquette, Stephen Boyd,
Adam Ford, linux-clk
Cc: kernel-janitors, linux-kernel
Hi Colin,
On 25/06/20 15:27, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There are a several places where printing an error message of
> init.name occurs after init.name has been kfree'd. Also the failure
> message is duplicated each time in the code. Fix this by adding
> a registration error failure path for these cases, moving the
> duplicated error messages to one common point and kfree'ing init.name
> only after it has been used.
>
> Changes also shrink the object code size by 171 bytes (x86-64, gcc 9.3):
>
> Before:
> text data bss dec hex filename
> 21057 3960 64 25081 61f9 drivers/clk/clk-versaclock5.o
>
> After:
> text data bss dec hex filename
> 20886 3960 64 24910 614e drivers/clk/clk-versaclock5.o
>
> Addresses-Coverity: ("Use after free")
> Fixes: f491276a5168 ("clk: vc5: Allow Versaclock driver to support multiple instances")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Woah, good catch!
> ---
> drivers/clk/clk-versaclock5.c | 51 +++++++++++++----------------------
> 1 file changed, 19 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index 9a5fb3834b9a..1d8ee4b8b1f5 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -882,11 +882,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
> init.parent_names = parent_names;
> vc5->clk_mux.init = &init;
> ret = devm_clk_hw_register(&client->dev, &vc5->clk_mux);
> + if (ret)
> + goto err_clk_register;
> kfree(init.name); /* clock framework made a copy of the name */
> - if (ret) {
> - dev_err(&client->dev, "unable to register %s\n", init.name);
> - goto err_clk;
> - }
>
> if (vc5->chip_info->flags & VC5_HAS_PFD_FREQ_DBL) {
> /* Register frequency doubler */
> @@ -900,12 +898,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
> init.num_parents = 1;
> vc5->clk_mul.init = &init;
> ret = devm_clk_hw_register(&client->dev, &vc5->clk_mul);
> + if (ret)
> + goto err_clk_register;
> kfree(init.name); /* clock framework made a copy of the name */
> - if (ret) {
> - dev_err(&client->dev, "unable to register %s\n",
> - init.name);
> - goto err_clk;
> - }
> }
>
> /* Register PFD */
> @@ -921,11 +916,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
> init.num_parents = 1;
> vc5->clk_pfd.init = &init;
> ret = devm_clk_hw_register(&client->dev, &vc5->clk_pfd);
> + if (ret)
> + goto err_clk_register;
> kfree(init.name); /* clock framework made a copy of the name */
> - if (ret) {
> - dev_err(&client->dev, "unable to register %s\n", init.name);
> - goto err_clk;
> - }
>
> /* Register PLL */
> memset(&init, 0, sizeof(init));
> @@ -939,11 +932,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
> vc5->clk_pll.vc5 = vc5;
> vc5->clk_pll.hw.init = &init;
> ret = devm_clk_hw_register(&client->dev, &vc5->clk_pll.hw);
> + if (ret)
> + goto err_clk_register;
> kfree(init.name); /* clock framework made a copy of the name */
> - if (ret) {
> - dev_err(&client->dev, "unable to register %s\n", init.name);
> - goto err_clk;
> - }
>
> /* Register FODs */
> for (n = 0; n < vc5->chip_info->clk_fod_cnt; n++) {
> @@ -960,12 +951,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
> vc5->clk_fod[n].vc5 = vc5;
> vc5->clk_fod[n].hw.init = &init;
> ret = devm_clk_hw_register(&client->dev, &vc5->clk_fod[n].hw);
> + if (ret)
> + goto err_clk_register;
> kfree(init.name); /* clock framework made a copy of the name */
> - if (ret) {
> - dev_err(&client->dev, "unable to register %s\n",
> - init.name);
> - goto err_clk;
> - }
> }
>
> /* Register MUX-connected OUT0_I2C_SELB output */
> @@ -981,11 +969,9 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
> vc5->clk_out[0].vc5 = vc5;
> vc5->clk_out[0].hw.init = &init;
> ret = devm_clk_hw_register(&client->dev, &vc5->clk_out[0].hw);
> - kfree(init.name); /* clock framework made a copy of the name */
> - if (ret) {
> - dev_err(&client->dev, "unable to register %s\n", init.name);
> - goto err_clk;
> - }
> + if (ret)
> + goto err_clk_register;
> + kfree(init.name); /* clock framework made a copy of the name */
>
> /* Register FOD-connected OUTx outputs */
> for (n = 1; n < vc5->chip_info->clk_out_cnt; n++) {
> @@ -1008,17 +994,15 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)
> vc5->clk_out[n].vc5 = vc5;
> vc5->clk_out[n].hw.init = &init;
> ret = devm_clk_hw_register(&client->dev, &vc5->clk_out[n].hw);
> + if (ret)
> + goto err_clk_register;
> kfree(init.name); /* clock framework made a copy of the name */
> - if (ret) {
> - dev_err(&client->dev, "unable to register %s\n",
> - init.name);
> - goto err_clk;
> - }
>
> /* Fetch Clock Output configuration from DT (if specified) */
> ret = vc5_get_output_config(client, &vc5->clk_out[n]);
> if (ret)
> goto err_clk;
> +
> }
Stray newline. With it possibly fixed:
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
--
Luca
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][next] clk: vc5: fix use of memory after it has been kfree'd
2020-06-25 13:27 [PATCH][next] clk: vc5: fix use of memory after it has been kfree'd Colin King
2020-07-21 10:10 ` Luca Ceresoli
@ 2020-07-23 1:46 ` Stephen Boyd
1 sibling, 0 replies; 3+ messages in thread
From: Stephen Boyd @ 2020-07-23 1:46 UTC (permalink / raw)
To: Adam Ford, Colin King, Marek Vasut, Michael Turquette, linux-clk
Cc: kernel-janitors, linux-kernel
Quoting Colin King (2020-06-25 06:27:36)
> From: Colin Ian King <colin.king@canonical.com>
>
> There are a several places where printing an error message of
> init.name occurs after init.name has been kfree'd. Also the failure
> message is duplicated each time in the code. Fix this by adding
> a registration error failure path for these cases, moving the
> duplicated error messages to one common point and kfree'ing init.name
> only after it has been used.
>
> Changes also shrink the object code size by 171 bytes (x86-64, gcc 9.3):
>
> Before:
> text data bss dec hex filename
> 21057 3960 64 25081 61f9 drivers/clk/clk-versaclock5.o
>
> After:
> text data bss dec hex filename
> 20886 3960 64 24910 614e drivers/clk/clk-versaclock5.o
>
> Addresses-Coverity: ("Use after free")
> Fixes: f491276a5168 ("clk: vc5: Allow Versaclock driver to support multiple instances")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
Applied to clk-next
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-23 1:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 13:27 [PATCH][next] clk: vc5: fix use of memory after it has been kfree'd Colin King
2020-07-21 10:10 ` Luca Ceresoli
2020-07-23 1:46 ` Stephen Boyd
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).