2014-06-27 10:34:37

by Krzysztof Kozlowski

[permalink] [raw]
Subject: [PATCH] clk: s2mps11: Fix double free corruption during driver unbind

After unbinding the driver memory was corrupted by double free of
clk_lookup structure. This lead to OOPS when re-binding the driver
again.

The driver allocated memory for 'clk_lookup' with devm_kzalloc. During
driver removal this memory was freed twice: once by clkdev_drop() and
second by devm code.

Kernel panic log:
[ 30.839284] Unable to handle kernel paging request at virtual address 5f343173
[ 30.846476] pgd = dee14000
[ 30.849165] [5f343173] *pgd=00000000
[ 30.852703] Internal error: Oops: 805 [#1] PREEMPT SMP ARM
[ 30.858166] Modules linked in:
[ 30.861208] CPU: 0 PID: 1 Comm: bash Not tainted 3.16.0-rc2-00239-g94bdf617b07e-dirty #40
[ 30.869364] task: df478000 ti: df480000 task.ti: df480000
[ 30.874752] PC is at clkdev_add+0x2c/0x38
[ 30.878738] LR is at clkdev_add+0x18/0x38
[ 30.882732] pc : [<c0350908>] lr : [<c03508f4>] psr: 60000013
[ 30.882732] sp : df481e78 ip : 00000001 fp : c0700ed8
[ 30.894187] r10: 0000000c r9 : 00000000 r8 : c07b0e3c
[ 30.899396] r7 : 00000002 r6 : df45f9d0 r5 : df421390 r4 : c0700d6c
[ 30.905906] r3 : 5f343173 r2 : c0700d84 r1 : 60000013 r0 : c0700d6c
[ 30.912417] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 30.919534] Control: 10c53c7d Table: 5ee1406a DAC: 00000015
[ 30.925262] Process bash (pid: 1, stack limit = 0xdf480240)
[ 30.930817] Stack: (0xdf481e78 to 0xdf482000)
[ 30.935159] 1e60: 00001000 df6de610
[ 30.943321] 1e80: df7f4558 c0355650 c05ec6ec c0700eb0 df6de600 df7f4510 dec9d69c 00000014
[ 30.951480] 1ea0: 00167b48 df6de610 c0700e30 c0713518 00000000 c0700e30 dec9d69c 00000006
[ 30.959639] 1ec0: 00167b48 c02c1b7c c02c1b64 df6de610 c07aff48 c02c0420 c06fb150 c047cc20
[ 30.967798] 1ee0: df6de610 df6de610 c0700e30 df6de644 c06fb150 0000000c dec9d690 c02bef90
[ 30.975957] 1f00: dec9c6c0 dece4c00 df481f80 dece4c00 0000000c c02be73c 0000000c c016ca8c
[ 30.984116] 1f20: c016ca48 00000000 00000000 c016c1f4 00000000 00000000 b6f18000 df481f80
[ 30.992276] 1f40: df7f66c0 0000000c df480000 df480000 b6f18000 c011094c df47839c 60000013
[ 31.000435] 1f60: 00000000 00000000 df7f66c0 df7f66c0 0000000c df480000 b6f18000 c0110dd4
[ 31.008594] 1f80: 00000000 00000000 0000000c b6ec05d8 0000000c b6f18000 00000004 c000f2a8
[ 31.016753] 1fa0: 00001000 c000f0e0 b6ec05d8 0000000c 00000001 b6f18000 0000000c 00000000
[ 31.024912] 1fc0: b6ec05d8 0000000c b6f18000 00000004 0000000c 00000001 00000000 00167b48
[ 31.033071] 1fe0: 00000000 bed83a80 b6e004f0 b6e5122c 60000010 00000001 ffffffff ffffffff
[ 31.041248] [<c0350908>] (clkdev_add) from [<c0355650>] (s2mps11_clk_probe+0x2b4/0x3b4)
[ 31.049223] [<c0355650>] (s2mps11_clk_probe) from [<c02c1b7c>] (platform_drv_probe+0x18/0x48)
[ 31.057728] [<c02c1b7c>] (platform_drv_probe) from [<c02c0420>] (driver_probe_device+0x13c/0x384)
[ 31.066579] [<c02c0420>] (driver_probe_device) from [<c02bef90>] (bind_store+0x88/0xd8)
[ 31.074564] [<c02bef90>] (bind_store) from [<c02be73c>] (drv_attr_store+0x20/0x2c)
[ 31.082118] [<c02be73c>] (drv_attr_store) from [<c016ca8c>] (sysfs_kf_write+0x44/0x48)
[ 31.090016] [<c016ca8c>] (sysfs_kf_write) from [<c016c1f4>] (kernfs_fop_write+0xc0/0x17c)
[ 31.098176] [<c016c1f4>] (kernfs_fop_write) from [<c011094c>] (vfs_write+0xa0/0x1c4)
[ 31.105899] [<c011094c>] (vfs_write) from [<c0110dd4>] (SyS_write+0x40/0x8c)
[ 31.112931] [<c0110dd4>] (SyS_write) from [<c000f0e0>] (ret_fast_syscall+0x0/0x3c)
[ 31.120481] Code: e2842018 e584501c e1a00004 e885000c (e5835000)
[ 31.126596] ---[ end trace efad45bfa3a61b05 ]---
[ 31.131181] Kernel panic - not syncing: Fatal exception
[ 31.136368] CPU1: stopping
[ 31.139054] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G D 3.16.0-rc2-00239-g94bdf617b07e-dirty #40
[ 31.148697] [<c0016480>] (unwind_backtrace) from [<c0012950>] (show_stack+0x10/0x14)
[ 31.156419] [<c0012950>] (show_stack) from [<c0480db8>] (dump_stack+0x80/0xcc)
[ 31.163622] [<c0480db8>] (dump_stack) from [<c001499c>] (handle_IPI+0x130/0x15c)
[ 31.170998] [<c001499c>] (handle_IPI) from [<c000862c>] (gic_handle_irq+0x60/0x68)
[ 31.178549] [<c000862c>] (gic_handle_irq) from [<c0013480>] (__irq_svc+0x40/0x70)
[ 31.186009] Exception stack(0xdf4bdf88 to 0xdf4bdfd0)
[ 31.191046] df80: ffffffed 00000000 00000000 00000000 df4bc000 c06d042c
[ 31.199207] dfa0: 00000000 ffffffed c06d03c0 00000000 c070c288 00000000 00000000 df4bdfd0
[ 31.207363] dfc0: c0010324 c0010328 60000013 ffffffff
[ 31.212402] [<c0013480>] (__irq_svc) from [<c0010328>] (arch_cpu_idle+0x28/0x30)
[ 31.219783] [<c0010328>] (arch_cpu_idle) from [<c005f150>] (cpu_startup_entry+0x2c4/0x3f0)
[ 31.228027] [<c005f150>] (cpu_startup_entry) from [<400086c4>] (0x400086c4)
[ 31.234968] ---[ end Kernel panic - not syncing: Fatal exception

Fixes: 7cc560dea415 ("clk: s2mps11: Add support for s2mps11")
Cc: <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/clk/clk-s2mps11.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
index 9b7b5859a420..27157f456ebe 100644
--- a/drivers/clk/clk-s2mps11.c
+++ b/drivers/clk/clk-s2mps11.c
@@ -230,8 +230,8 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
goto err_reg;
}

- s2mps11_clk->lookup = devm_kzalloc(&pdev->dev,
- sizeof(struct clk_lookup), GFP_KERNEL);
+ s2mps11_clk->lookup = kzalloc(sizeof(struct clk_lookup),
+ GFP_KERNEL);
if (!s2mps11_clk->lookup) {
ret = -ENOMEM;
goto err_lup;
--
1.9.1


2014-06-27 11:49:26

by Yadwinder Singh Brar

[permalink] [raw]
Subject: Re: [PATCH] clk: s2mps11: Fix double free corruption during driver unbind

Hi Krzystof,

On Fri, Jun 27, 2014 at 4:04 PM, Krzysztof Kozlowski
<[email protected]> wrote:
> After unbinding the driver memory was corrupted by double free of
> clk_lookup structure. This lead to OOPS when re-binding the driver
> again.
>
> The driver allocated memory for 'clk_lookup' with devm_kzalloc. During
> driver removal this memory was freed twice: once by clkdev_drop() and
> second by devm code.
>

Ideally memory should be allocated and deallocated in same place
either driver or framework, so in this case if framework(clkdev_drop)
deallocates memory, framework itself should also allocate it.
So IMO this bug should be fixed using clkdev_alloc() instead of kzalloc().

Regards,
Yadwinder

> Kernel panic log:
> [ 30.839284] Unable to handle kernel paging request at virtual address 5f343173
> [ 30.846476] pgd = dee14000
> [ 30.849165] [5f343173] *pgd=00000000
> [ 30.852703] Internal error: Oops: 805 [#1] PREEMPT SMP ARM
> [ 30.858166] Modules linked in:
> [ 30.861208] CPU: 0 PID: 1 Comm: bash Not tainted 3.16.0-rc2-00239-g94bdf617b07e-dirty #40
> [ 30.869364] task: df478000 ti: df480000 task.ti: df480000
> [ 30.874752] PC is at clkdev_add+0x2c/0x38
> [ 30.878738] LR is at clkdev_add+0x18/0x38
> [ 30.882732] pc : [<c0350908>] lr : [<c03508f4>] psr: 60000013
> [ 30.882732] sp : df481e78 ip : 00000001 fp : c0700ed8
> [ 30.894187] r10: 0000000c r9 : 00000000 r8 : c07b0e3c
> [ 30.899396] r7 : 00000002 r6 : df45f9d0 r5 : df421390 r4 : c0700d6c
> [ 30.905906] r3 : 5f343173 r2 : c0700d84 r1 : 60000013 r0 : c0700d6c
> [ 30.912417] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> [ 30.919534] Control: 10c53c7d Table: 5ee1406a DAC: 00000015
> [ 30.925262] Process bash (pid: 1, stack limit = 0xdf480240)
> [ 30.930817] Stack: (0xdf481e78 to 0xdf482000)
> [ 30.935159] 1e60: 00001000 df6de610
> [ 30.943321] 1e80: df7f4558 c0355650 c05ec6ec c0700eb0 df6de600 df7f4510 dec9d69c 00000014
> [ 30.951480] 1ea0: 00167b48 df6de610 c0700e30 c0713518 00000000 c0700e30 dec9d69c 00000006
> [ 30.959639] 1ec0: 00167b48 c02c1b7c c02c1b64 df6de610 c07aff48 c02c0420 c06fb150 c047cc20
> [ 30.967798] 1ee0: df6de610 df6de610 c0700e30 df6de644 c06fb150 0000000c dec9d690 c02bef90
> [ 30.975957] 1f00: dec9c6c0 dece4c00 df481f80 dece4c00 0000000c c02be73c 0000000c c016ca8c
> [ 30.984116] 1f20: c016ca48 00000000 00000000 c016c1f4 00000000 00000000 b6f18000 df481f80
> [ 30.992276] 1f40: df7f66c0 0000000c df480000 df480000 b6f18000 c011094c df47839c 60000013
> [ 31.000435] 1f60: 00000000 00000000 df7f66c0 df7f66c0 0000000c df480000 b6f18000 c0110dd4
> [ 31.008594] 1f80: 00000000 00000000 0000000c b6ec05d8 0000000c b6f18000 00000004 c000f2a8
> [ 31.016753] 1fa0: 00001000 c000f0e0 b6ec05d8 0000000c 00000001 b6f18000 0000000c 00000000
> [ 31.024912] 1fc0: b6ec05d8 0000000c b6f18000 00000004 0000000c 00000001 00000000 00167b48
> [ 31.033071] 1fe0: 00000000 bed83a80 b6e004f0 b6e5122c 60000010 00000001 ffffffff ffffffff
> [ 31.041248] [<c0350908>] (clkdev_add) from [<c0355650>] (s2mps11_clk_probe+0x2b4/0x3b4)
> [ 31.049223] [<c0355650>] (s2mps11_clk_probe) from [<c02c1b7c>] (platform_drv_probe+0x18/0x48)
> [ 31.057728] [<c02c1b7c>] (platform_drv_probe) from [<c02c0420>] (driver_probe_device+0x13c/0x384)
> [ 31.066579] [<c02c0420>] (driver_probe_device) from [<c02bef90>] (bind_store+0x88/0xd8)
> [ 31.074564] [<c02bef90>] (bind_store) from [<c02be73c>] (drv_attr_store+0x20/0x2c)
> [ 31.082118] [<c02be73c>] (drv_attr_store) from [<c016ca8c>] (sysfs_kf_write+0x44/0x48)
> [ 31.090016] [<c016ca8c>] (sysfs_kf_write) from [<c016c1f4>] (kernfs_fop_write+0xc0/0x17c)
> [ 31.098176] [<c016c1f4>] (kernfs_fop_write) from [<c011094c>] (vfs_write+0xa0/0x1c4)
> [ 31.105899] [<c011094c>] (vfs_write) from [<c0110dd4>] (SyS_write+0x40/0x8c)
> [ 31.112931] [<c0110dd4>] (SyS_write) from [<c000f0e0>] (ret_fast_syscall+0x0/0x3c)
> [ 31.120481] Code: e2842018 e584501c e1a00004 e885000c (e5835000)
> [ 31.126596] ---[ end trace efad45bfa3a61b05 ]---
> [ 31.131181] Kernel panic - not syncing: Fatal exception
> [ 31.136368] CPU1: stopping
> [ 31.139054] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G D 3.16.0-rc2-00239-g94bdf617b07e-dirty #40
> [ 31.148697] [<c0016480>] (unwind_backtrace) from [<c0012950>] (show_stack+0x10/0x14)
> [ 31.156419] [<c0012950>] (show_stack) from [<c0480db8>] (dump_stack+0x80/0xcc)
> [ 31.163622] [<c0480db8>] (dump_stack) from [<c001499c>] (handle_IPI+0x130/0x15c)
> [ 31.170998] [<c001499c>] (handle_IPI) from [<c000862c>] (gic_handle_irq+0x60/0x68)
> [ 31.178549] [<c000862c>] (gic_handle_irq) from [<c0013480>] (__irq_svc+0x40/0x70)
> [ 31.186009] Exception stack(0xdf4bdf88 to 0xdf4bdfd0)
> [ 31.191046] df80: ffffffed 00000000 00000000 00000000 df4bc000 c06d042c
> [ 31.199207] dfa0: 00000000 ffffffed c06d03c0 00000000 c070c288 00000000 00000000 df4bdfd0
> [ 31.207363] dfc0: c0010324 c0010328 60000013 ffffffff
> [ 31.212402] [<c0013480>] (__irq_svc) from [<c0010328>] (arch_cpu_idle+0x28/0x30)
> [ 31.219783] [<c0010328>] (arch_cpu_idle) from [<c005f150>] (cpu_startup_entry+0x2c4/0x3f0)
> [ 31.228027] [<c005f150>] (cpu_startup_entry) from [<400086c4>] (0x400086c4)
> [ 31.234968] ---[ end Kernel panic - not syncing: Fatal exception
>
> Fixes: 7cc560dea415 ("clk: s2mps11: Add support for s2mps11")
> Cc: <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> drivers/clk/clk-s2mps11.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
> index 9b7b5859a420..27157f456ebe 100644
> --- a/drivers/clk/clk-s2mps11.c
> +++ b/drivers/clk/clk-s2mps11.c
> @@ -230,8 +230,8 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
> goto err_reg;
> }
>
> - s2mps11_clk->lookup = devm_kzalloc(&pdev->dev,
> - sizeof(struct clk_lookup), GFP_KERNEL);
> + s2mps11_clk->lookup = kzalloc(sizeof(struct clk_lookup),
> + GFP_KERNEL);
> if (!s2mps11_clk->lookup) {
> ret = -ENOMEM;
> goto err_lup;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>

2014-06-27 12:03:18

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH] clk: s2mps11: Fix double free corruption during driver unbind

On piÄ…, 2014-06-27 at 17:19 +0530, Yadwinder Singh Brar wrote:
> Hi Krzystof,
>
> On Fri, Jun 27, 2014 at 4:04 PM, Krzysztof Kozlowski
> <[email protected]> wrote:
> > After unbinding the driver memory was corrupted by double free of
> > clk_lookup structure. This lead to OOPS when re-binding the driver
> > again.
> >
> > The driver allocated memory for 'clk_lookup' with devm_kzalloc. During
> > driver removal this memory was freed twice: once by clkdev_drop() and
> > second by devm code.
> >
>
> Ideally memory should be allocated and deallocated in same place
> either driver or framework, so in this case if framework(clkdev_drop)
> deallocates memory, framework itself should also allocate it.
> So IMO this bug should be fixed using clkdev_alloc() instead of kzalloc().
>

You're right, I'll send fixed version. Thanks for idea.

Best regards,
Krzysztof