[PATCH 0/3] clk: zynqmp: Misc fixes

[PATCH 0/3] clk: zynqmp: Misc fixes

[Shubhrajyoti Datta]

The series fixes
- Memory leak 
- A stack-out-of-bounds in strncpy
- Adds a check for return type

Ian Nam (1):
  clk: zynqmp: Fix stack-out-of-bounds in strncpy`

Shubhrajyoti Datta (2):
  clk: zynqmp: Check the return type
  clk: zynqmp: Fix a memory leak

 drivers/clk/zynqmp/clk-mux-zynqmp.c |  2 +-
 drivers/clk/zynqmp/clkc.c           | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH 1/3] clk: zynqmp: Check the return type

[Shubhrajyoti Datta]

Currently the return value of of_clk_add_hw_provider is ignored.
lets check and return value.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/clk/zynqmp/clkc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index 871184e406e1..eb25303eefed 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -762,9 +762,7 @@ static int zynqmp_clk_setup(struct device_node *np)
 	zynqmp_register_clocks(np);
 
 	zynqmp_data->num = clock_max_idx;
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, zynqmp_data);
-
-	return 0;
+	return of_clk_add_hw_provider(np, of_clk_hw_onecell_get, zynqmp_data);
 }
 
 static int zynqmp_clock_probe(struct platform_device *pdev)
-- 
2.17.1

Re: [PATCH 1/3] clk: zynqmp: Check the return type

[Stephen Boyd]

Quoting Shubhrajyoti Datta (2021-08-17 23:59:27)
> Currently the return value of of_clk_add_hw_provider is ignored.
> lets check and return value.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---

Applied to clk-next

Re: [PATCH 1/3] clk: zynqmp: Check the return type

[Stephen Boyd]

Quoting Shubhrajyoti Datta (2021-08-17 23:59:27)
> Currently the return value of of_clk_add_hw_provider is ignored.
> lets check and return value.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---

Applied to clk-next

[PATCH 2/3] clk: zynqmp: Fix a memory leak

[Shubhrajyoti Datta]

Fix a memory leak of mux.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/clk/zynqmp/clk-mux-zynqmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/zynqmp/clk-mux-zynqmp.c b/drivers/clk/zynqmp/clk-mux-zynqmp.c
index 157d4a960bf4..17afce594f28 100644
--- a/drivers/clk/zynqmp/clk-mux-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-mux-zynqmp.c
@@ -159,7 +159,7 @@ struct clk_hw *zynqmp_clk_register_mux(const char *name, u32 clk_id,
 	hw = &mux->hw;
 	ret = clk_hw_register(NULL, hw);
 	if (ret) {
-		kfree(hw);
+		kfree(mux);
 		hw = ERR_PTR(ret);
 	}
 
-- 
2.17.1

Re: [PATCH 2/3] clk: zynqmp: Fix a memory leak

[Stephen Boyd]

Quoting Shubhrajyoti Datta (2021-08-17 23:59:28)
> Fix a memory leak of mux.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---

Applied to clk-next

[PATCH 3/3] clk: zynqmp: Fix stack-out-of-bounds in strncpy`

[Shubhrajyoti Datta]

From: Ian Nam <young.kwan.nam@xilinx.com>

"BUG: KASAN: stack-out-of-bounds in strncpy+0x30/0x68"

Linux-ATF interface is using 16 bytes of SMC payload. In case clock name is
longer than 15 bytes, string terminated NULL character will not be received
by Linux. Add explicit NULL character at last byte to fix issues when clock
name is longer.

This fixes below bug reported by KASAN:

[    7.522474] ==================================================================
[    7.529795] BUG: KASAN: stack-out-of-bounds in strncpy+0x30/0x68
[    7.535871] Read of size 1 at addr ffff0008c89a7410 by task swapper/0/1
[    7.542557]
[    7.544065] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.4.0-00396-g81ef9e7-dirty #3
[    7.551809] Hardware name: Xilinx Versal vck190 Eval board revA (QSPI) (DT)
[    7.558847] Call trace:
[    7.561321]  dump_backtrace+0x0/0x1e8
[    7.565023]  show_stack+0x14/0x20
[    7.568374]  dump_stack+0xd4/0x108
[    7.571817]  print_address_description.isra.0+0xbc/0x37c
[    7.577189]  __kasan_report+0x144/0x198
[    7.581068]  kasan_report+0xc/0x18
[    7.584507]  __asan_load1+0x5c/0x68
[    7.588032]  strncpy+0x30/0x68
[    7.591120]  zynqmp_clock_probe+0x238/0x7b8
[    7.595350]  platform_drv_probe+0x6c/0xc8
[    7.599405]  really_probe+0x14c/0x418
[    7.603108]  driver_probe_device+0x74/0x130
[    7.607339]  __device_attach_driver+0xc4/0xe8
[    7.611744]  bus_for_each_drv+0xec/0x150
[    7.615711]  __device_attach+0x160/0x1d8
[    7.619678]  device_initial_probe+0x10/0x18
[    7.623907]  bus_probe_device+0xe0/0xf0
[    7.627785]  device_add+0x528/0x950
[    7.631312]  of_device_add+0x5c/0x80
[    7.634926]  of_platform_device_create_pdata+0x120/0x168
[    7.640299]  of_platform_bus_create+0x244/0x4e0
[    7.644880]  of_platform_populate+0x50/0xe8
[    7.649110]  zynqmp_firmware_probe+0x370/0x3a8
[    7.653602]  platform_drv_probe+0x6c/0xc8
[    7.657656]  really_probe+0x14c/0x418
[    7.661359]  driver_probe_device+0x74/0x130
[    7.665589]  device_driver_attach+0x94/0xa0
[    7.669820]  __driver_attach+0x70/0x108
[    7.673698]  bus_for_each_dev+0xe4/0x158
[    7.677664]  driver_attach+0x30/0x40
[    7.681278]  bus_add_driver+0x21c/0x2b8
[    7.685157]  driver_register+0xbc/0x1d0
[    7.689035]  __platform_driver_register+0x7c/0x88
[    7.693793]  zynqmp_firmware_driver_init+0x1c/0x24
[    7.698637]  do_one_initcall+0xa4/0x234
[    7.702518]  kernel_init_freeable+0x1b0/0x24c
[    7.706924]  kernel_init+0x10/0x110
[    7.710450]  ret_from_fork+0x10/0x18
[    7.714058]
[    7.715559] The buggy address belongs to the page:
[    7.720405] page:ffff0008f9be1c88 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0
[    7.728772] raw: 0008d00000000000 ffff0008f9be1c90 ffff0008f9be1c90 0000000000000000
[    7.736606] raw: 0000000000000000 0000000000000000 00000000ffffffff
[    7.742942] page dumped because: kasan: bad access detected
[    7.748572]
[    7.750076] addr ffff0008c89a7410 is located in stack of task swapper/0/1 at offset 112 in frame:
[    7.759052]  zynqmp_clock_probe+0x0/0x7b8
[    7.763103]
[    7.764604] this frame has 3 objects:
[    7.768306]  [32, 44) 'response'
[    7.768312]  [64, 80) 'ret_payload'
[    7.771573]  [96, 112) 'name'
[    7.775095]
[    7.779585] Memory state around the buggy address:
[    7.784430]  ffff0008c89a7300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    7.791735]  ffff0008c89a7380: 00 00 00 00 f1 f1 f1 f1 00 04 f2 f2 00 00 f2 f2
[    7.799040] >ffff0008c89a7400: 00 00 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
[    7.806342]                          ^
[    7.810132]  ffff0008c89a7480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    7.817437]  ffff0008c89a7500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    7.824738] ==================================================================

Signed-off-by: Ian Nam <young.kwan.nam@xilinx.com>
Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/clk/zynqmp/clkc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index eb25303eefed..2c9da6623b84 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -710,6 +710,13 @@ static void zynqmp_get_clock_info(void)
 				  FIELD_PREP(CLK_ATTR_NODE_INDEX, i);
 
 		zynqmp_pm_clock_get_name(clock[i].clk_id, &name);
+
+		/*
+		 * Terminate with NULL character in case name provided by firmware
+		 * is longer and truncated due to size limit.
+		 */
+		name.name[sizeof(name.name) - 1] = '\0';
+
 		if (!strcmp(name.name, RESERVED_CLK_NAME))
 			continue;
 		strncpy(clock[i].clk_name, name.name, MAX_NAME_LEN);
-- 
2.17.1

Re: [PATCH 3/3] clk: zynqmp: Fix stack-out-of-bounds in strncpy`

[Stephen Boyd]

Quoting Shubhrajyoti Datta (2021-08-17 23:59:29)
> From: Ian Nam <young.kwan.nam@xilinx.com>
> 
> "BUG: KASAN: stack-out-of-bounds in strncpy+0x30/0x68"
> 
> Linux-ATF interface is using 16 bytes of SMC payload. In case clock name is
> longer than 15 bytes, string terminated NULL character will not be received
> by Linux. Add explicit NULL character at last byte to fix issues when clock
> name is longer.
> 
> This fixes below bug reported by KASAN:
> 
> [    7.522474] ==================================================================
> [    7.529795] BUG: KASAN: stack-out-of-bounds in strncpy+0x30/0x68
> [    7.535871] Read of size 1 at addr ffff0008c89a7410 by task swapper/0/1
> [    7.542557]
> [    7.544065] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.4.0-00396-g81ef9e7-dirty #3
> [    7.551809] Hardware name: Xilinx Versal vck190 Eval board revA (QSPI) (DT)
> [    7.558847] Call trace:
> [    7.561321]  dump_backtrace+0x0/0x1e8
> [    7.565023]  show_stack+0x14/0x20
> [    7.568374]  dump_stack+0xd4/0x108
> [    7.571817]  print_address_description.isra.0+0xbc/0x37c
> [    7.577189]  __kasan_report+0x144/0x198
> [    7.581068]  kasan_report+0xc/0x18
> [    7.584507]  __asan_load1+0x5c/0x68
> [    7.588032]  strncpy+0x30/0x68
> [    7.591120]  zynqmp_clock_probe+0x238/0x7b8
> [    7.595350]  platform_drv_probe+0x6c/0xc8
> [    7.599405]  really_probe+0x14c/0x418
> [    7.603108]  driver_probe_device+0x74/0x130
> [    7.607339]  __device_attach_driver+0xc4/0xe8
> [    7.611744]  bus_for_each_drv+0xec/0x150
> [    7.615711]  __device_attach+0x160/0x1d8
> [    7.619678]  device_initial_probe+0x10/0x18
> [    7.623907]  bus_probe_device+0xe0/0xf0
> [    7.627785]  device_add+0x528/0x950
> [    7.631312]  of_device_add+0x5c/0x80
> [    7.634926]  of_platform_device_create_pdata+0x120/0x168
> [    7.640299]  of_platform_bus_create+0x244/0x4e0
> [    7.644880]  of_platform_populate+0x50/0xe8
> [    7.649110]  zynqmp_firmware_probe+0x370/0x3a8
> [    7.653602]  platform_drv_probe+0x6c/0xc8
> [    7.657656]  really_probe+0x14c/0x418
> [    7.661359]  driver_probe_device+0x74/0x130
> [    7.665589]  device_driver_attach+0x94/0xa0
> [    7.669820]  __driver_attach+0x70/0x108
> [    7.673698]  bus_for_each_dev+0xe4/0x158
> [    7.677664]  driver_attach+0x30/0x40
> [    7.681278]  bus_add_driver+0x21c/0x2b8
> [    7.685157]  driver_register+0xbc/0x1d0
> [    7.689035]  __platform_driver_register+0x7c/0x88
> [    7.693793]  zynqmp_firmware_driver_init+0x1c/0x24
> [    7.698637]  do_one_initcall+0xa4/0x234
> [    7.702518]  kernel_init_freeable+0x1b0/0x24c
> [    7.706924]  kernel_init+0x10/0x110
> [    7.710450]  ret_from_fork+0x10/0x18
> [    7.714058]
> [    7.715559] The buggy address belongs to the page:
> [    7.720405] page:ffff0008f9be1c88 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0
> [    7.728772] raw: 0008d00000000000 ffff0008f9be1c90 ffff0008f9be1c90 0000000000000000
> [    7.736606] raw: 0000000000000000 0000000000000000 00000000ffffffff
> [    7.742942] page dumped because: kasan: bad access detected
> [    7.748572]
> [    7.750076] addr ffff0008c89a7410 is located in stack of task swapper/0/1 at offset 112 in frame:
> [    7.759052]  zynqmp_clock_probe+0x0/0x7b8
> [    7.763103]
> [    7.764604] this frame has 3 objects:
> [    7.768306]  [32, 44) 'response'
> [    7.768312]  [64, 80) 'ret_payload'
> [    7.771573]  [96, 112) 'name'
> [    7.775095]
> [    7.779585] Memory state around the buggy address:
> [    7.784430]  ffff0008c89a7300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [    7.791735]  ffff0008c89a7380: 00 00 00 00 f1 f1 f1 f1 00 04 f2 f2 00 00 f2 f2
> [    7.799040] >ffff0008c89a7400: 00 00 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
> [    7.806342]                          ^
> [    7.810132]  ffff0008c89a7480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [    7.817437]  ffff0008c89a7500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [    7.824738] ==================================================================
> 
> Signed-off-by: Ian Nam <young.kwan.nam@xilinx.com>
> Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
>  drivers/clk/zynqmp/clkc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index eb25303eefed..2c9da6623b84 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -710,6 +710,13 @@ static void zynqmp_get_clock_info(void)
>                                   FIELD_PREP(CLK_ATTR_NODE_INDEX, i);
>  
>                 zynqmp_pm_clock_get_name(clock[i].clk_id, &name);
> +
> +               /*
> +                * Terminate with NULL character in case name provided by firmware
> +                * is longer and truncated due to size limit.
> +                */
> +               name.name[sizeof(name.name) - 1] = '\0';
> +
>                 if (!strcmp(name.name, RESERVED_CLK_NAME))
>                         continue;
>                 strncpy(clock[i].clk_name, name.name, MAX_NAME_LEN);

Why not use strscpy()?