clk: samsung: exynos5433: Fix potential NULL pointer dereference

Message ID 20191001130921.24571-1-m.szyprowski@samsung.com
State New
Headers show
Series
  • clk: samsung: exynos5433: Fix potential NULL pointer dereference
Related show

Commit Message

Marek Szyprowski Oct. 1, 2019, 1:09 p.m.
devm_kcalloc might fail, so avoid accessing the allocated object in such
case.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/clk/samsung/clk-exynos5433.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Krzysztof Kozlowski Oct. 1, 2019, 1:16 p.m. | #1
On Tue, Oct 01, 2019 at 03:09:21PM +0200, Marek Szyprowski wrote:
> devm_kcalloc might fail, so avoid accessing the allocated object in such

> case.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/clk/samsung/clk-exynos5433.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c

> index 7824c2ba3d8e..6afbcd0ae96f 100644

> --- a/drivers/clk/samsung/clk-exynos5433.c

> +++ b/drivers/clk/samsung/clk-exynos5433.c

> @@ -5592,7 +5592,8 @@ static int __init exynos5433_cmu_probe(struct platform_device *pdev)

>  	if (data->nr_pclks > 0) {

>  		data->pclks = devm_kcalloc(dev, sizeof(struct clk *),

>  					   data->nr_pclks, GFP_KERNEL);

> -

> +		if (!data->pclks)

> +			return -ENOMEM;


You leak the memory from the samsung_clk_alloc_reg_dump() call.

The error path few lines later (from of_clk_get()) leaks it as well.

Best regards,
Krzysztof

>  		for (i = 0; i < data->nr_pclks; i++) {

>  			struct clk *clk = of_clk_get(dev->of_node, i);

>  

> -- 

> 2.17.1

>
Bartlomiej Zolnierkiewicz Oct. 1, 2019, 1:22 p.m. | #2
On 10/1/19 3:16 PM, Krzysztof Kozlowski wrote:
> On Tue, Oct 01, 2019 at 03:09:21PM +0200, Marek Szyprowski wrote:

>> devm_kcalloc might fail, so avoid accessing the allocated object in such

>> case.

>>

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> ---

>>  drivers/clk/samsung/clk-exynos5433.c | 3 ++-

>>  1 file changed, 2 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c

>> index 7824c2ba3d8e..6afbcd0ae96f 100644

>> --- a/drivers/clk/samsung/clk-exynos5433.c

>> +++ b/drivers/clk/samsung/clk-exynos5433.c

>> @@ -5592,7 +5592,8 @@ static int __init exynos5433_cmu_probe(struct platform_device *pdev)

>>  	if (data->nr_pclks > 0) {

>>  		data->pclks = devm_kcalloc(dev, sizeof(struct clk *),

>>  					   data->nr_pclks, GFP_KERNEL);

>> -

>> +		if (!data->pclks)

>> +			return -ENOMEM;

> 

> You leak the memory from the samsung_clk_alloc_reg_dump() call.


Also we may want to fix the code to check samsung_clk_alloc_reg_dump()
return value for NULL while we are at it..

> The error path few lines later (from of_clk_get()) leaks it as well.

> 

> Best regards,

> Krzysztof

> 

>>  		for (i = 0; i < data->nr_pclks; i++) {

>>  			struct clk *clk = of_clk_get(dev->of_node, i);

>>  

>> -- 

>> 2.17.1

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Chanwoo Choi Oct. 1, 2019, 10:03 p.m. | #3
Hi,

On 19. 10. 1. 오후 10:09, Marek Szyprowski wrote:
> devm_kcalloc might fail, so avoid accessing the allocated object in such

> case.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/clk/samsung/clk-exynos5433.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c

> index 7824c2ba3d8e..6afbcd0ae96f 100644

> --- a/drivers/clk/samsung/clk-exynos5433.c

> +++ b/drivers/clk/samsung/clk-exynos5433.c

> @@ -5592,7 +5592,8 @@ static int __init exynos5433_cmu_probe(struct platform_device *pdev)

>  	if (data->nr_pclks > 0) {

>  		data->pclks = devm_kcalloc(dev, sizeof(struct clk *),

>  					   data->nr_pclks, GFP_KERNEL);

> -

> +		if (!data->pclks)

> +			return -ENOMEM;

>  		for (i = 0; i < data->nr_pclks; i++) {

>  			struct clk *clk = of_clk_get(dev->of_node, i);

>  

> 


I think it is needed when 'data->pclks' memory allocation failed.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Patch

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 7824c2ba3d8e..6afbcd0ae96f 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -5592,7 +5592,8 @@  static int __init exynos5433_cmu_probe(struct platform_device *pdev)
 	if (data->nr_pclks > 0) {
 		data->pclks = devm_kcalloc(dev, sizeof(struct clk *),
 					   data->nr_pclks, GFP_KERNEL);
-
+		if (!data->pclks)
+			return -ENOMEM;
 		for (i = 0; i < data->nr_pclks; i++) {
 			struct clk *clk = of_clk_get(dev->of_node, i);