[RESEND PATCH v2 1/4] dmaengine: pl330: Remove non-NULL check for pl330_submit_req parameters

[RESEND PATCH v2 1/4] dmaengine: pl330: Remove non-NULL check for pl330_submit_req parameters

[Krzysztof Kozlowski]

The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and
'struct dma_pl330_desc desc' parameters for non-NULL. However these
checks are useless because supplied arguments won't be NULL.

The pl330_submit_req() is called in only one place and:
1. 'desc' is already dereferenced in fill_queue() before calling
   pl330_submit_req().
2. 'thrd' is always dereferenced after calling
   fill_queue()->pl330_submit_req().

Removing the checks for non-NULL values fixes following warning:
drivers/dma/pl330.c:1376 pl330_submit_req() warn: variable dereferenced before check 'thrd' (see line 1367)

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Changes since v1:
=================
1. Remove the checks for non-NULL completely instead of moving them
   before dereference. Suggested by Lars-Peter Clausen.
---
 drivers/dma/pl330.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index d5149aacd2fe..57049f84d0c0 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1372,10 +1372,6 @@ static int pl330_submit_req(struct pl330_thread *thrd,
 	u32 ccr;
 	int ret = 0;
 
-	/* No Req or Unacquired Channel or DMAC */
-	if (!desc || !thrd || thrd->free)
-		return -EINVAL;
-
 	regs = thrd->dmac->base;
 
 	if (pl330->state == DYING
-- 
1.9.1

[RESEND PATCH v2 2/4] dmaengine: pl330: Remove unused 'regs' variable in pl330_submit_req()

[Krzysztof Kozlowski]

The 'void __iomem *regs' is not used in pl330_submit_req() function.
Remove it.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Changes since v1:
=================
1. New patch.
---
 drivers/dma/pl330.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 57049f84d0c0..28e3775888a6 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1367,13 +1367,10 @@ static int pl330_submit_req(struct pl330_thread *thrd,
 	struct pl330_dmac *pl330 = thrd->dmac;
 	struct _xfer_spec xs;
 	unsigned long flags;
-	void __iomem *regs;
 	unsigned idx;
 	u32 ccr;
 	int ret = 0;
 
-	regs = thrd->dmac->base;
-
 	if (pl330->state == DYING
 		|| pl330->dmac_tbd.reset_chan & (1 << thrd->id)) {
 		dev_info(thrd->dmac->ddma.dev, "%s:%d\n",
-- 
1.9.1

Re: [RESEND PATCH v2 2/4] dmaengine: pl330: Remove unused 'regs' variable in pl330_submit_req()

[Vinod Koul]

On Mon, Sep 29, 2014 at 02:42:19PM +0200, Krzysztof Kozlowski wrote:
> The 'void __iomem *regs' is not used in pl330_submit_req() function.
> Remove it.
Applied, thanks


-- 
~Vinod

> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> ---
> 
> Changes since v1:
> =================
> 1. New patch.
> ---
>  drivers/dma/pl330.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 57049f84d0c0..28e3775888a6 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -1367,13 +1367,10 @@ static int pl330_submit_req(struct pl330_thread *thrd,
>  	struct pl330_dmac *pl330 = thrd->dmac;
>  	struct _xfer_spec xs;
>  	unsigned long flags;
> -	void __iomem *regs;
>  	unsigned idx;
>  	u32 ccr;
>  	int ret = 0;
>  
> -	regs = thrd->dmac->base;
> -
>  	if (pl330->state == DYING
>  		|| pl330->dmac_tbd.reset_chan & (1 << thrd->id)) {
>  		dev_info(thrd->dmac->ddma.dev, "%s:%d\n",
> -- 
> 1.9.1
> 

-- 

[RESEND PATCH v2 3/4] dmaengine: pl330: Fix NULL pointer dereference on probe failure

[Krzysztof Kozlowski]

If dma_async_device_register() returns error and probe should clean up
and return error, a NULL pointer exception happens because of
dereference of not allocated channel thread:

Dmesg log (from early printk):
dma-pl330 12680000.pdma: unable to register DMAC
DMA pl330_control: removing pch: eeac4000, chan: eeac4014, thread:   (null)
Unable to handle kernel NULL pointer dereference at virtual address 0000000c
pgd = c0004000
[0000000c] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 2 PID: 1 Comm: swapper/0 Not tainted 3.17.0-rc3-next-20140904-00005-g6cc4c1937d90-dirty #427
task: ee80a800 ti: ee888000 task.ti: ee888000
PC is at _stop+0x8/0x2c8
LR is at pl330_control+0x70/0x2e8
pc : [<c0205dc8>]    lr : [<c020623c>]    psr: 60000193
sp : ee889df8  ip : 00000002  fp : 00000000
r10: eeac4014  r9 : ee0e62bc  r8 : 00000000
r7 : eeac405c  r6 : 60000113  r5 : ee0e6210  r4 : eeac4000
r3 : 00000002  r2 : 00000002  r1 : 00010000  r0 : 00000000
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 4000404a  DAC: 00000015
Process swapper/0 (pid: 1, stack limit = 0xee888240)
Stack: (0xee889df8 to 0xee88a000)
9de0:                                                       00000002 eeac4000
9e00: ee0e6210 eeac4000 ee0e6210 60000113 eeac405c c020623c 00000000 c020725c
9e20: ee889e20 ee889e20 ee0e6210 eeac4080 00200200 00100100 eeac4014 00000020
9e40: ee0e6218 c0208374 00000000 ee9bb340 ee0e6210 00000000 00000000 c0605cd8
9e60: ee970000 c0605c84 ee9700f8 00000000 c05c4270 00000000 00000000 c0203b3c
9e80: ee970000 c06624a8 00000000 c0605c84 00000000 c023f890 ee970000 c0605c84
9ea0: ee970034 00000000 c05b23d0 c023fa3c 00000000 c0605c84 c023f9b0 c023e0d4
9ec0: ee947e78 ee9b9440 c0605c84 eea1e780 c0605acc c023f094 c0513b50 c0605c84
9ee0: c05ecbd8 c0605c84 c05ecbd8 ee11ba40 c0626500 c0240064 00000000 c05ecbd8
9f00: c05ecbd8 c0008964 c040f13c 0000009f c0626500 c057465c ee80a800 60000113
9f20: 00000000 c05efdb0 60000113 00000000 ef7fc89d c0421168 0000008f c003787c
9f40: c0573d6c 00000006 ef7fc8bb 00000006 c05efd50 ef7fc800 c05dfbc4 00000006
9f60: c05c4264 c0626500 0000008f c05c4270 c059b518 c059bcb4 00000006 00000006
9f80: c059b518 c003c08c 00000000 c040091c 00000000 00000000 00000000 00000000
9fa0: 00000000 c0400924 00000000 c000e7b8 00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 c0c0c0c0 c0c0c0c0
[<c0205dc8>] (_stop) from [<c020623c>] (pl330_control+0x70/0x2e8)
[<c020623c>] (pl330_control) from [<c0208374>] (pl330_probe+0x594/0x75c)
[<c0208374>] (pl330_probe) from [<c0203b3c>] (amba_probe+0xb8/0x120)
[<c0203b3c>] (amba_probe) from [<c023f890>] (driver_probe_device+0x10c/0x22c)
[<c023f890>] (driver_probe_device) from [<c023fa3c>] (__driver_attach+0x8c/0x90)
[<c023fa3c>] (__driver_attach) from [<c023e0d4>] (bus_for_each_dev+0x54/0x88)
[<c023e0d4>] (bus_for_each_dev) from [<c023f094>] (bus_add_driver+0xd4/0x1d0)
[<c023f094>] (bus_add_driver) from [<c0240064>] (driver_register+0x78/0xf4)
[<c0240064>] (driver_register) from [<c0008964>] (do_one_initcall+0x80/0x1d0)
[<c0008964>] (do_one_initcall) from [<c059bcb4>] (kernel_init_freeable+0x108/0x1d4)
[<c059bcb4>] (kernel_init_freeable) from [<c0400924>] (kernel_init+0x8/0xec)
[<c0400924>] (kernel_init) from [<c000e7b8>] (ret_from_fork+0x14/0x3c)
Code: e5813010 e12fff1e e92d40f0 e24dd00c (e590200c)
---[ end trace c94b2f4f38dff3bf ]---

This happens because the necessary resources were not yet allocated - no
call to pl330_alloc_chan_resources().

Terminate the thread and free channel resource only if channel thread is not NULL.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: <stable@vger.kernel.org>
Fixes: 0b94c5771705 ("DMA: PL330: Add check if device tree compatible")
Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

---

Changes since v1:
=================
1. Add Lars-Peter Clausen's reviewed-by tag.
---
 drivers/dma/pl330.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 28e3775888a6..4a2caaa0432e 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2748,8 +2748,10 @@ probe_err3:
 		list_del(&pch->chan.device_node);
 
 		/* Flush the channel */
-		pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
-		pl330_free_chan_resources(&pch->chan);
+		if (pch->thread) {
+			pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
+			pl330_free_chan_resources(&pch->chan);
+		}
 	}
 probe_err2:
 	pl330_del(pl330);
-- 
1.9.1

Re: [RESEND PATCH v2 3/4] dmaengine: pl330: Fix NULL pointer dereference on probe failure

[Vinod Koul]

On Mon, Sep 29, 2014 at 02:42:20PM +0200, Krzysztof Kozlowski wrote:
> If dma_async_device_register() returns error and probe should clean up
> and return error, a NULL pointer exception happens because of
> dereference of not allocated channel thread:
> 
> Dmesg log (from early printk):
> dma-pl330 12680000.pdma: unable to register DMAC
> DMA pl330_control: removing pch: eeac4000, chan: eeac4014, thread:   (null)
> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> pgd = c0004000
> [0000000c] *pgd=00000000
> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 2 PID: 1 Comm: swapper/0 Not tainted 3.17.0-rc3-next-20140904-00005-g6cc4c1937d90-dirty #427
> task: ee80a800 ti: ee888000 task.ti: ee888000
> PC is at _stop+0x8/0x2c8
> LR is at pl330_control+0x70/0x2e8
> pc : [<c0205dc8>]    lr : [<c020623c>]    psr: 60000193
> sp : ee889df8  ip : 00000002  fp : 00000000
> r10: eeac4014  r9 : ee0e62bc  r8 : 00000000
> r7 : eeac405c  r6 : 60000113  r5 : ee0e6210  r4 : eeac4000
> r3 : 00000002  r2 : 00000002  r1 : 00010000  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> Control: 10c5387d  Table: 4000404a  DAC: 00000015
> Process swapper/0 (pid: 1, stack limit = 0xee888240)
> Stack: (0xee889df8 to 0xee88a000)
> 9de0:                                                       00000002 eeac4000
> 9e00: ee0e6210 eeac4000 ee0e6210 60000113 eeac405c c020623c 00000000 c020725c
> 9e20: ee889e20 ee889e20 ee0e6210 eeac4080 00200200 00100100 eeac4014 00000020
> 9e40: ee0e6218 c0208374 00000000 ee9bb340 ee0e6210 00000000 00000000 c0605cd8
> 9e60: ee970000 c0605c84 ee9700f8 00000000 c05c4270 00000000 00000000 c0203b3c
> 9e80: ee970000 c06624a8 00000000 c0605c84 00000000 c023f890 ee970000 c0605c84
> 9ea0: ee970034 00000000 c05b23d0 c023fa3c 00000000 c0605c84 c023f9b0 c023e0d4
> 9ec0: ee947e78 ee9b9440 c0605c84 eea1e780 c0605acc c023f094 c0513b50 c0605c84
> 9ee0: c05ecbd8 c0605c84 c05ecbd8 ee11ba40 c0626500 c0240064 00000000 c05ecbd8
> 9f00: c05ecbd8 c0008964 c040f13c 0000009f c0626500 c057465c ee80a800 60000113
> 9f20: 00000000 c05efdb0 60000113 00000000 ef7fc89d c0421168 0000008f c003787c
> 9f40: c0573d6c 00000006 ef7fc8bb 00000006 c05efd50 ef7fc800 c05dfbc4 00000006
> 9f60: c05c4264 c0626500 0000008f c05c4270 c059b518 c059bcb4 00000006 00000006
> 9f80: c059b518 c003c08c 00000000 c040091c 00000000 00000000 00000000 00000000
> 9fa0: 00000000 c0400924 00000000 c000e7b8 00000000 00000000 00000000 00000000
> 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 c0c0c0c0 c0c0c0c0
> [<c0205dc8>] (_stop) from [<c020623c>] (pl330_control+0x70/0x2e8)
> [<c020623c>] (pl330_control) from [<c0208374>] (pl330_probe+0x594/0x75c)
> [<c0208374>] (pl330_probe) from [<c0203b3c>] (amba_probe+0xb8/0x120)
> [<c0203b3c>] (amba_probe) from [<c023f890>] (driver_probe_device+0x10c/0x22c)
> [<c023f890>] (driver_probe_device) from [<c023fa3c>] (__driver_attach+0x8c/0x90)
> [<c023fa3c>] (__driver_attach) from [<c023e0d4>] (bus_for_each_dev+0x54/0x88)
> [<c023e0d4>] (bus_for_each_dev) from [<c023f094>] (bus_add_driver+0xd4/0x1d0)
> [<c023f094>] (bus_add_driver) from [<c0240064>] (driver_register+0x78/0xf4)
> [<c0240064>] (driver_register) from [<c0008964>] (do_one_initcall+0x80/0x1d0)
> [<c0008964>] (do_one_initcall) from [<c059bcb4>] (kernel_init_freeable+0x108/0x1d4)
> [<c059bcb4>] (kernel_init_freeable) from [<c0400924>] (kernel_init+0x8/0xec)
> [<c0400924>] (kernel_init) from [<c000e7b8>] (ret_from_fork+0x14/0x3c)
> Code: e5813010 e12fff1e e92d40f0 e24dd00c (e590200c)
> ---[ end trace c94b2f4f38dff3bf ]---
> 
> This happens because the necessary resources were not yet allocated - no
> call to pl330_alloc_chan_resources().
> 
> Terminate the thread and free channel resource only if channel thread is not NULL.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: <stable@vger.kernel.org>
> Fixes: 0b94c5771705 ("DMA: PL330: Add check if device tree compatible")
> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

Applied, thanks

-- 
~Vinod

> 
> ---
> 
> Changes since v1:
> =================
> 1. Add Lars-Peter Clausen's reviewed-by tag.
> ---
>  drivers/dma/pl330.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 28e3775888a6..4a2caaa0432e 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2748,8 +2748,10 @@ probe_err3:
>  		list_del(&pch->chan.device_node);
>  
>  		/* Flush the channel */
> -		pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
> -		pl330_free_chan_resources(&pch->chan);
> +		if (pch->thread) {
> +			pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
> +			pl330_free_chan_resources(&pch->chan);
> +		}
>  	}
>  probe_err2:
>  	pl330_del(pl330);
> -- 
> 1.9.1
> 

-- 

[RESEND PATCH v2 4/4] dmaengine: pl330: Fix NULL pointer dereference on driver unbind

[Krzysztof Kozlowski]

Fix a NULL pointer dereference after unbinding the driver, if channel
resources were not yet allocated (no call to
pl330_alloc_chan_resources()):
$ echo 12850000.mdma > /sys/bus/amba/drivers/dma-pl330/unbind
[   13.606533] DMA pl330_control: removing pch: eeab6800, chan: eeab6814, thread:   (null)
[   13.614472] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
[   13.622537] pgd = ee284000
[   13.625228] [0000000c] *pgd=6e1e4831, *pte=00000000, *ppte=00000000
[   13.631482] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
[   13.636859] Modules linked in:
[   13.639903] CPU: 0 PID: 1 Comm: sh Not tainted 3.17.0-rc3-next-20140904-00004-g7020ffc33ca3-dirty #420
[   13.649187] task: ee80a800 ti: ee888000 task.ti: ee888000
[   13.654589] PC is at _stop+0x8/0x2c8
[   13.658131] LR is at pl330_control+0x70/0x2e8
[   13.662468] pc : [<c0206028>]    lr : [<c020649c>]    psr: 60000093
[   13.662468] sp : ee889e58  ip : 00000001  fp : 000bab70
[   13.673922] r10: eeab6814  r9 : ee16debc  r8 : 00000000
[   13.679131] r7 : eeab685c  r6 : 60000013  r5 : ee16de10  r4 : eeab6800
[   13.685641] r3 : 00000002  r2 : 00000000  r1 : 00010000  r0 : 00000000
[   13.692153] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   13.699357] Control: 10c5387d  Table: 6e28404a  DAC: 00000015
[   13.705085] Process sh (pid: 1, stack limit = 0xee888240)
[   13.710466] Stack: (0xee889e58 to 0xee88a000)
[   13.714808] 9e40:                                                       00000002 eeab6800
[   13.722969] 9e60: ee16de10 eeab6800 ee16de10 60000013 eeab685c c020649c 00000000 c040280c
[   13.731128] 9e80: ee889e80 ee889e80 ee16de18 ee16de10 eeab6880 eeab6814 00200200 eeab68a8
[   13.739287] 9ea0: 00100100 c0208048 00000000 c0409fc4 eea80800 eea808f8 c0605c44 0000000e
[   13.747446] 9ec0: 0000000e eeb3960c eeb39600 c0203c48 eea80800 c0605c44 c0605a8c c023f694
[   13.755605] 9ee0: ee80a800 eea80834 eea80800 c023f704 ee80a800 eea80800 c0605c44 c023e8ec
[   13.763764] 9f00: 0000000e ee149780 ee29e580 ee889f80 ee29e580 c023e19c 0000000e c01167e4
[   13.771923] 9f20: c01167a0 00000000 00000000 c0115e88 00000000 00000000 ee0b1a00 0000000e
[   13.780082] 9f40: b6f48000 ee889f80 0000000e ee888000 b6f48000 c00bfadc 00000000 00000003
[   13.788241] 9f60: 00000000 00000000 00000000 ee0b1a00 ee0b1a00 0000000e b6f48000 c00bfdf4
[   13.796401] 9f80: 00000000 00000000 ffffffff 0000000e b6f48000 b6edc5d0 00000004 c000e7a4
[   13.804560] 9fa0: 00000000 c000e620 0000000e b6f48000 00000001 b6f48000 0000000e 00000000
[   13.812719] 9fc0: 0000000e b6f48000 b6edc5d0 00000004 0000000e b6f4c8c0 000c3470 000bab70
[   13.820879] 9fe0: 00000000 bed2aa50 b6e18bdc b6e6b52c 60000010 00000001 c0c0c0c0 c0c0c0c0
[   13.829058] [<c0206028>] (_stop) from [<c020649c>] (pl330_control+0x70/0x2e8)
[   13.836165] [<c020649c>] (pl330_control) from [<c0208048>] (pl330_remove+0xb0/0xdc)
[   13.843800] [<c0208048>] (pl330_remove) from [<c0203c48>] (amba_remove+0x24/0xc0)
[   13.851272] [<c0203c48>] (amba_remove) from [<c023f694>] (__device_release_driver+0x70/0xc4)
[   13.859685] [<c023f694>] (__device_release_driver) from [<c023f704>] (device_release_driver+0x1c/0x28)
[   13.868971] [<c023f704>] (device_release_driver) from [<c023e8ec>] (unbind_store+0x58/0x90)
[   13.877303] [<c023e8ec>] (unbind_store) from [<c023e19c>] (drv_attr_store+0x20/0x2c)
[   13.885036] [<c023e19c>] (drv_attr_store) from [<c01167e4>] (sysfs_kf_write+0x44/0x48)
[   13.892928] [<c01167e4>] (sysfs_kf_write) from [<c0115e88>] (kernfs_fop_write+0xc0/0x17c)
[   13.901090] [<c0115e88>] (kernfs_fop_write) from [<c00bfadc>] (vfs_write+0xa0/0x1a8)
[   13.908812] [<c00bfadc>] (vfs_write) from [<c00bfdf4>] (SyS_write+0x40/0x8c)
[   13.915850] [<c00bfdf4>] (SyS_write) from [<c000e620>] (ret_fast_syscall+0x0/0x30)
[   13.923392] Code: e5813010 e12fff1e e92d40f0 e24dd00c (e590200c)
[   13.929467] ---[ end trace 10064e15a5929cf8 ]---

Terminate the thread and free channel resource only if channel resources
were allocated (thread is not NULL).

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: <stable@vger.kernel.org>
Fixes: b3040e40675e ("DMA: PL330: Add dma api driver")
Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

---

Changes since v1:
=================
1. Add Lars-Peter Clausen's reviewed-by tag.
---
 drivers/dma/pl330.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4a2caaa0432e..4839bfa74a10 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2777,8 +2777,10 @@ static int pl330_remove(struct amba_device *adev)
 		list_del(&pch->chan.device_node);
 
 		/* Flush the channel */
-		pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
-		pl330_free_chan_resources(&pch->chan);
+		if (pch->thread) {
+			pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
+			pl330_free_chan_resources(&pch->chan);
+		}
 	}
 
 	pl330_del(pl330);
-- 
1.9.1

Re: [RESEND PATCH v2 4/4] dmaengine: pl330: Fix NULL pointer dereference on driver unbind

[Vinod Koul]

On Mon, Sep 29, 2014 at 02:42:21PM +0200, Krzysztof Kozlowski wrote:
> Fix a NULL pointer dereference after unbinding the driver, if channel
> resources were not yet allocated (no call to
> pl330_alloc_chan_resources()):
> $ echo 12850000.mdma > /sys/bus/amba/drivers/dma-pl330/unbind
> [   13.606533] DMA pl330_control: removing pch: eeab6800, chan: eeab6814, thread:   (null)
> [   13.614472] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> [   13.622537] pgd = ee284000
> [   13.625228] [0000000c] *pgd=6e1e4831, *pte=00000000, *ppte=00000000
> [   13.631482] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
> [   13.636859] Modules linked in:
> [   13.639903] CPU: 0 PID: 1 Comm: sh Not tainted 3.17.0-rc3-next-20140904-00004-g7020ffc33ca3-dirty #420
> [   13.649187] task: ee80a800 ti: ee888000 task.ti: ee888000
> [   13.654589] PC is at _stop+0x8/0x2c8
> [   13.658131] LR is at pl330_control+0x70/0x2e8
> [   13.662468] pc : [<c0206028>]    lr : [<c020649c>]    psr: 60000093
> [   13.662468] sp : ee889e58  ip : 00000001  fp : 000bab70
> [   13.673922] r10: eeab6814  r9 : ee16debc  r8 : 00000000
> [   13.679131] r7 : eeab685c  r6 : 60000013  r5 : ee16de10  r4 : eeab6800
> [   13.685641] r3 : 00000002  r2 : 00000000  r1 : 00010000  r0 : 00000000
> [   13.692153] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> [   13.699357] Control: 10c5387d  Table: 6e28404a  DAC: 00000015
> [   13.705085] Process sh (pid: 1, stack limit = 0xee888240)
> [   13.710466] Stack: (0xee889e58 to 0xee88a000)
> [   13.714808] 9e40:                                                       00000002 eeab6800
> [   13.722969] 9e60: ee16de10 eeab6800 ee16de10 60000013 eeab685c c020649c 00000000 c040280c
> [   13.731128] 9e80: ee889e80 ee889e80 ee16de18 ee16de10 eeab6880 eeab6814 00200200 eeab68a8
> [   13.739287] 9ea0: 00100100 c0208048 00000000 c0409fc4 eea80800 eea808f8 c0605c44 0000000e
> [   13.747446] 9ec0: 0000000e eeb3960c eeb39600 c0203c48 eea80800 c0605c44 c0605a8c c023f694
> [   13.755605] 9ee0: ee80a800 eea80834 eea80800 c023f704 ee80a800 eea80800 c0605c44 c023e8ec
> [   13.763764] 9f00: 0000000e ee149780 ee29e580 ee889f80 ee29e580 c023e19c 0000000e c01167e4
> [   13.771923] 9f20: c01167a0 00000000 00000000 c0115e88 00000000 00000000 ee0b1a00 0000000e
> [   13.780082] 9f40: b6f48000 ee889f80 0000000e ee888000 b6f48000 c00bfadc 00000000 00000003
> [   13.788241] 9f60: 00000000 00000000 00000000 ee0b1a00 ee0b1a00 0000000e b6f48000 c00bfdf4
> [   13.796401] 9f80: 00000000 00000000 ffffffff 0000000e b6f48000 b6edc5d0 00000004 c000e7a4
> [   13.804560] 9fa0: 00000000 c000e620 0000000e b6f48000 00000001 b6f48000 0000000e 00000000
> [   13.812719] 9fc0: 0000000e b6f48000 b6edc5d0 00000004 0000000e b6f4c8c0 000c3470 000bab70
> [   13.820879] 9fe0: 00000000 bed2aa50 b6e18bdc b6e6b52c 60000010 00000001 c0c0c0c0 c0c0c0c0
> [   13.829058] [<c0206028>] (_stop) from [<c020649c>] (pl330_control+0x70/0x2e8)
> [   13.836165] [<c020649c>] (pl330_control) from [<c0208048>] (pl330_remove+0xb0/0xdc)
> [   13.843800] [<c0208048>] (pl330_remove) from [<c0203c48>] (amba_remove+0x24/0xc0)
> [   13.851272] [<c0203c48>] (amba_remove) from [<c023f694>] (__device_release_driver+0x70/0xc4)
> [   13.859685] [<c023f694>] (__device_release_driver) from [<c023f704>] (device_release_driver+0x1c/0x28)
> [   13.868971] [<c023f704>] (device_release_driver) from [<c023e8ec>] (unbind_store+0x58/0x90)
> [   13.877303] [<c023e8ec>] (unbind_store) from [<c023e19c>] (drv_attr_store+0x20/0x2c)
> [   13.885036] [<c023e19c>] (drv_attr_store) from [<c01167e4>] (sysfs_kf_write+0x44/0x48)
> [   13.892928] [<c01167e4>] (sysfs_kf_write) from [<c0115e88>] (kernfs_fop_write+0xc0/0x17c)
> [   13.901090] [<c0115e88>] (kernfs_fop_write) from [<c00bfadc>] (vfs_write+0xa0/0x1a8)
> [   13.908812] [<c00bfadc>] (vfs_write) from [<c00bfdf4>] (SyS_write+0x40/0x8c)
> [   13.915850] [<c00bfdf4>] (SyS_write) from [<c000e620>] (ret_fast_syscall+0x0/0x30)
> [   13.923392] Code: e5813010 e12fff1e e92d40f0 e24dd00c (e590200c)
> [   13.929467] ---[ end trace 10064e15a5929cf8 ]---
> 
> Terminate the thread and free channel resource only if channel resources
> were allocated (thread is not NULL).
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: <stable@vger.kernel.org>
> Fixes: b3040e40675e ("DMA: PL330: Add dma api driver")
> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
Applied, thanks

-- 
~Vinod


> 
> ---
> 
> Changes since v1:
> =================
> 1. Add Lars-Peter Clausen's reviewed-by tag.
> ---
>  drivers/dma/pl330.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 4a2caaa0432e..4839bfa74a10 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2777,8 +2777,10 @@ static int pl330_remove(struct amba_device *adev)
>  		list_del(&pch->chan.device_node);
>  
>  		/* Flush the channel */
> -		pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
> -		pl330_free_chan_resources(&pch->chan);
> +		if (pch->thread) {
> +			pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
> +			pl330_free_chan_resources(&pch->chan);
> +		}
>  	}
>  
>  	pl330_del(pl330);
> -- 
> 1.9.1
> 

-- 

Re: [RESEND PATCH v2 1/4] dmaengine: pl330: Remove non-NULL check for pl330_submit_req parameters

[Vinod Koul]

On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote:
> The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and
> 'struct dma_pl330_desc desc' parameters for non-NULL. However these
> checks are useless because supplied arguments won't be NULL.
even if we have some error or bug?

I would like this to be checked and complained loudly so we know something
is going wrong rather than assuming it will be correct always.

-- 
~Vinod
> 
> The pl330_submit_req() is called in only one place and:
> 1. 'desc' is already dereferenced in fill_queue() before calling
>    pl330_submit_req().
> 2. 'thrd' is always dereferenced after calling
>    fill_queue()->pl330_submit_req().
> 
> Removing the checks for non-NULL values fixes following warning:
> drivers/dma/pl330.c:1376 pl330_submit_req() warn: variable dereferenced before check 'thrd' (see line 1367)
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> ---
> 
> Changes since v1:
> =================
> 1. Remove the checks for non-NULL completely instead of moving them
>    before dereference. Suggested by Lars-Peter Clausen.
> ---
>  drivers/dma/pl330.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index d5149aacd2fe..57049f84d0c0 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -1372,10 +1372,6 @@ static int pl330_submit_req(struct pl330_thread *thrd,
>  	u32 ccr;
>  	int ret = 0;
>  
> -	/* No Req or Unacquired Channel or DMAC */
> -	if (!desc || !thrd || thrd->free)
> -		return -EINVAL;
> -
>  	regs = thrd->dmac->base;
>  
>  	if (pl330->state == DYING
> -- 
> 1.9.1
> 

-- 

Re: [RESEND PATCH v2 1/4] dmaengine: pl330: Remove non-NULL check for pl330_submit_req parameters

[Lars-Peter Clausen]

On 10/14/2014 01:51 PM, Vinod Koul wrote:
> On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote:
>> The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and
>> 'struct dma_pl330_desc desc' parameters for non-NULL. However these
>> checks are useless because supplied arguments won't be NULL.
> even if we have some error or bug?
>
> I would like this to be checked and complained loudly so we know something
> is going wrong rather than assuming it will be correct always.
>

In this case it is impossible to get to this code portion if the pointer is 
NULL because it would have crashed earlier.

- Lars

Re: [RESEND PATCH v2 1/4] dmaengine: pl330: Remove non-NULL check for pl330_submit_req parameters

[Vinod Koul]

On Tue, Oct 14, 2014 at 02:40:09PM +0200, Lars-Peter Clausen wrote:
> On 10/14/2014 01:51 PM, Vinod Koul wrote:
> >On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote:
> >>The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and
> >>'struct dma_pl330_desc desc' parameters for non-NULL. However these
> >>checks are useless because supplied arguments won't be NULL.
> >even if we have some error or bug?
> >
> >I would like this to be checked and complained loudly so we know something
> >is going wrong rather than assuming it will be correct always.
> >
> 
> In this case it is impossible to get to this code portion if the
> pointer is NULL because it would have crashed earlier.
Yes checking again,
for desc it seems not neccessary as its deref earlier in calling code and
thrd shouldnt be NULL, so will apply now

-- 
~Vinod

Re: [RESEND PATCH v2 1/4] dmaengine: pl330: Remove non-NULL check for pl330_submit_req parameters

[Krzysztof Kozlowski]

On wto, 2014-10-14 at 17:21 +0530, Vinod Koul wrote:
> On Mon, Sep 29, 2014 at 02:42:18PM +0200, Krzysztof Kozlowski wrote:
> > The pl330_submit_req() checked supplied 'struct pl330_thread thrd' and
> > 'struct dma_pl330_desc desc' parameters for non-NULL. However these
> > checks are useless because supplied arguments won't be NULL.
> even if we have some error or bug?
> 
> I would like this to be checked and complained loudly so we know something
> is going wrong rather than assuming it will be correct always.

Currently the driver would fail before or after, regardless of this
check. However I do not insist on this approach.

The first version of patch was little different:
https://lkml.org/lkml/2014/9/5/390

Do you think that 1st version is worth resending?

Best regards,
Krzysztof