All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch -target tree] usb: gadget: f_tcm: use after free
@ 2016-03-02 10:08 Dan Carpenter
  2016-03-02 11:55 ` Felipe Balbi
  2016-03-05  7:20 ` Nicholas A. Bellinger
  0 siblings, 2 replies; 10+ messages in thread
From: Dan Carpenter @ 2016-03-02 10:08 UTC (permalink / raw)
  To: Felipe Balbi, Christoph Hellwig, Nicholas Bellinger
  Cc: Greg Kroah-Hartman, Sebastian Andrzej Siewior,
	Andrzej Pietrasiewicz, Bart Van Assche, linux-usb, linux-kernel,
	target-devel

We need to move the kfree() down a line so we don't dereference a freed
variable.

Fixes: 1b418a8fcbc0 ('target: Convert demo-mode only drivers to target_alloc_session')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 7276a73..e352a31 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1596,8 +1596,8 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
 #define MAKE_NEXUS_MSG "core_tpg_check_initiator_node_acl() failed for %s\n"
 		pr_debug(MAKE_NEXUS_MSG, name);
 #undef MAKE_NEXUS_MSG
-		kfree(tv_nexus);
 		ret = PTR_ERR(tv_nexus->tvn_se_sess);
+		kfree(tv_nexus);
 	}
 
 out_unlock:

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-02 10:08 [patch -target tree] usb: gadget: f_tcm: use after free Dan Carpenter
@ 2016-03-02 11:55 ` Felipe Balbi
  2016-03-05  7:26   ` Nicholas A. Bellinger
  2016-03-05  7:20 ` Nicholas A. Bellinger
  1 sibling, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2016-03-02 11:55 UTC (permalink / raw)
  To: Dan Carpenter, Christoph Hellwig, Nicholas Bellinger
  Cc: Greg Kroah-Hartman, Sebastian Andrzej Siewior,
	Andrzej Pietrasiewicz, Bart Van Assche, linux-usb, linux-kernel,
	target-devel

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> We need to move the kfree() down a line so we don't dereference a freed
> variable.
>
> Fixes: 1b418a8fcbc0 ('target: Convert demo-mode only drivers to target_alloc_session')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

It's okay to take this via target:

Signed-off-by: Felipe Balbi <balbi@kernel.org>

> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 7276a73..e352a31 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1596,8 +1596,8 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
>  #define MAKE_NEXUS_MSG "core_tpg_check_initiator_node_acl() failed for %s\n"
>  		pr_debug(MAKE_NEXUS_MSG, name);
>  #undef MAKE_NEXUS_MSG
> -		kfree(tv_nexus);
>  		ret = PTR_ERR(tv_nexus->tvn_se_sess);
> +		kfree(tv_nexus);
>  	}
>  
>  out_unlock:

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-02 10:08 [patch -target tree] usb: gadget: f_tcm: use after free Dan Carpenter
  2016-03-02 11:55 ` Felipe Balbi
@ 2016-03-05  7:20 ` Nicholas A. Bellinger
  1 sibling, 0 replies; 10+ messages in thread
From: Nicholas A. Bellinger @ 2016-03-05  7:20 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Felipe Balbi, Christoph Hellwig, Greg Kroah-Hartman,
	Sebastian Andrzej Siewior, Andrzej Pietrasiewicz,
	Bart Van Assche, linux-usb, linux-kernel, target-devel

On Wed, 2016-03-02 at 13:08 +0300, Dan Carpenter wrote:
> We need to move the kfree() down a line so we don't dereference a freed
> variable.
> 
> Fixes: 1b418a8fcbc0 ('target: Convert demo-mode only drivers to target_alloc_session')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 7276a73..e352a31 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1596,8 +1596,8 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
>  #define MAKE_NEXUS_MSG "core_tpg_check_initiator_node_acl() failed for %s\n"
>  		pr_debug(MAKE_NEXUS_MSG, name);
>  #undef MAKE_NEXUS_MSG
> -		kfree(tv_nexus);
>  		ret = PTR_ERR(tv_nexus->tvn_se_sess);
> +		kfree(tv_nexus);
>  	}
>  
>  out_unlock:

Fixed + squashed into the original patch.

Thanks Dan.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-02 11:55 ` Felipe Balbi
@ 2016-03-05  7:26   ` Nicholas A. Bellinger
  2016-03-09 11:38     ` Felipe Balbi
  2016-03-09 12:53     ` Andrzej Pietrasiewicz
  0 siblings, 2 replies; 10+ messages in thread
From: Nicholas A. Bellinger @ 2016-03-05  7:26 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Dan Carpenter, Christoph Hellwig, Greg Kroah-Hartman,
	Sebastian Andrzej Siewior, Andrzej Pietrasiewicz,
	Bart Van Assche, linux-usb, linux-kernel, target-devel

Hi Felipe + usb-gadget folks,

On Wed, 2016-03-02 at 13:55 +0200, Felipe Balbi wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> > We need to move the kfree() down a line so we don't dereference a freed
> > variable.
> >
> > Fixes: 1b418a8fcbc0 ('target: Convert demo-mode only drivers to target_alloc_session')
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> It's okay to take this via target:
> 
> Signed-off-by: Felipe Balbi <balbi@kernel.org>
> 

Note this specific patch is only a mechanical change, and we still need
reviews for the more interesting conversions:

usb-gadget/tcm: Conversion to percpu_ida tag pre-allocation
http://www.spinics.net/lists/target-devel/msg11777.html

usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs
http://www.spinics.net/lists/target-devel/msg11782.html

Felipe, Sebastian, & Andrezj, would you be so kind to review and test
usb-gadget using target-pending/for-next code..?

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-05  7:26   ` Nicholas A. Bellinger
@ 2016-03-09 11:38     ` Felipe Balbi
  2016-03-10  6:23       ` Nicholas A. Bellinger
  2016-03-09 12:53     ` Andrzej Pietrasiewicz
  1 sibling, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2016-03-09 11:38 UTC (permalink / raw)
  To: Nicholas A. Bellinger
  Cc: Dan Carpenter, Christoph Hellwig, Greg Kroah-Hartman,
	Sebastian Andrzej Siewior, Andrzej Pietrasiewicz,
	Bart Van Assche, linux-usb, linux-kernel, target-devel

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]


Hi,

"Nicholas A. Bellinger" <nab@linux-iscsi.org> writes:
> [ text/plain ]
> Hi Felipe + usb-gadget folks,
>
> On Wed, 2016-03-02 at 13:55 +0200, Felipe Balbi wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> > We need to move the kfree() down a line so we don't dereference a freed
>> > variable.
>> >
>> > Fixes: 1b418a8fcbc0 ('target: Convert demo-mode only drivers to target_alloc_session')
>> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> 
>> It's okay to take this via target:
>> 
>> Signed-off-by: Felipe Balbi <balbi@kernel.org>
>> 
>
> Note this specific patch is only a mechanical change, and we still need
> reviews for the more interesting conversions:
>
> usb-gadget/tcm: Conversion to percpu_ida tag pre-allocation
> http://www.spinics.net/lists/target-devel/msg11777.html
>
> usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs
> http://www.spinics.net/lists/target-devel/msg11782.html
>
I don't have either patch in my inbox apparently. Care to resend ?

sorry

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-05  7:26   ` Nicholas A. Bellinger
  2016-03-09 11:38     ` Felipe Balbi
@ 2016-03-09 12:53     ` Andrzej Pietrasiewicz
  2016-03-10  5:19       ` Nicholas A. Bellinger
  1 sibling, 1 reply; 10+ messages in thread
From: Andrzej Pietrasiewicz @ 2016-03-09 12:53 UTC (permalink / raw)
  To: Nicholas A. Bellinger, Felipe Balbi
  Cc: Dan Carpenter, Christoph Hellwig, Greg Kroah-Hartman,
	Sebastian Andrzej Siewior, Bart Van Assche, linux-usb,
	linux-kernel, target-devel

Hi Nicholas,

W dniu 05.03.2016 o 08:26, Nicholas A. Bellinger pisze:
> Hi Felipe + usb-gadget folks,
>
> On Wed, 2016-03-02 at 13:55 +0200, Felipe Balbi wrote:

<snip>

>
> usb-gadget/tcm: Conversion to percpu_ida tag pre-allocation
> http://www.spinics.net/lists/target-devel/msg11777.html
>
> usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs
> http://www.spinics.net/lists/target-devel/msg11782.html
>
> Felipe, Sebastian, & Andrezj, would you be so kind to review and test
> usb-gadget using target-pending/for-next code..?
>
>

Actually I have noticed a problem at
8b00965 "target: Convert demo-mode only drivers to target_alloc_session"

I get:

[ 1698.406304] configfs-gadget gadget: high-speed config #1: c
[ 1698.410547] Using the BOT protocol
[ 1698.414163] Missing nexus, ignoring command
[ 1698.417944] bot_cmd_complete(309): -22

I think the third message is from f_tcm. It is because
tpg->tpg_nexus is not set anymore, because the line

	tpg->tpg_nexus = tv_nexus;

is removed by the commit mentioned above.

Restoring this line fixes the problem.

Then percpu_ida commit produces the below result
(it does not happen immediately, but a while after
running the script).
I did not bisect, though; I only checked the commits
which alter drivers/usb/gadget/function/f_tcm.c.
The last one which (almost) works is:

8b00965 "target: Convert demo-mode only drivers to target_alloc_session"

AP

I used the following script:

8<----------------------------

#!/bin/bash

mount -t configfs none /sys/kernel/config
modprobe libcomposite
cd /sys/kernel/config/usb_gadget
mkdir tcm
cd tcm
mkdir functions/tcm.0
cd /sys/kernel/config/target/
mkdir usb_gadget
cd usb_gadget
mkdir naa.0123456789abcdef
cd naa.0123456789abcdef
mkdir tpgt_1
cd tpgt_1
echo naa.01234567890abcdef > nexus
echo 1 > enable
cd /sys/kernel/config/usb_gadget/tcm
mkdir configs/c.1
ln -s functions/tcm.0 configs/c.1
echo 0x0525 > idVendor
echo 0x1234 > idProduct
echo 12400000.dwc3 > UDC

8<----------------------------

# [   45.510513] ERR bot_status_complete(73)
[   45.671921] configfs-gadget gadget: high-speed config #1: c
[   45.676158] Using the BOT protocol
[   45.679860] ------------[ cut here ]------------
[   45.683981] kernel BUG at drivers/target/target_core_transport.c:1476!
[   45.690480] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[   45.696285] Modules linked in: usb_f_tcm libcomposite
[   45.701316] CPU: 2 PID: 1221 Comm: kworker/2:1 Not tainted 4.5.0-rc5+ #171
[   45.708156] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   45.714258] Workqueue: tcm_usb_gadget bot_cmd_work [usb_f_tcm]
[   45.720030] task: edc1f6c0 ti: ee2d6000 task.ti: ee2d6000
[   45.725410] PC is at target_submit_cmd_map_sgls+0x1fc/0x20c
[   45.730947] LR is at 0xeea71400
[   45.734070] pc : [<c0326a00>]    lr : [<eea71400>]    psr: a0000013
[   45.734070] sp : ee2d7e50  ip : 00000000  fp : c07f8100
[   45.745505] r10: 00000000  r9 : eefa25c0  r8 : ed593988
[   45.750705] r7 : ed593b8c  r6 : ee00d830  r5 : 00000000  r4 : ed5939e0
[   45.757204] r3 : bf0144d8  r2 : ed593988  r1 : eea71400  r0 : ed5939e0
[   45.763705] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   45.770809] Control: 10c5387d  Table: 6d44806a  DAC: 00000051
[   45.776528] Process kworker/2:1 (pid: 1221, stack limit = 0xee2d6210)
[   45.782940] Stack: (0xee2d7e50 to 0xee2d8000)
[   45.787276] 7e40:                                     002293a6 00000000 00000000 00000000
[   45.795423] 7e60: 00000000 00000000 00000003 00000020 00000000 c0326a60 00000000 00000000
[   45.803568] 7e80: 00000000 00000020 00000003 00000000 00000000 00000000 00000000 00000000
[   45.811714] 7ea0: 00000000 00000000 ed5939cc 00000020 ed5939e0 edf6b580 00000000 bf012358
[   45.819860] 7ec0: 00000000 00000000 00000000 00000020 00000003 00000000 eefa2ac0 ee30da00
[   45.828006] 7ee0: ed5939cc eefa25c0 eefaaa00 c00369bc edc1f6c0 ee30da00 00000001 ee30da00
[   45.836151] 7f00: eefa25e4 00000001 eefa25c0 ee30da18 eefa25c0 00000008 c07f8100 c0036c24
[   45.844297] 7f20: edc1f6c0 eeafefc0 00000000 eeafefc0 00000000 ee30da00 c0036bf8 00000000
[   45.852442] 7f40: 00000000 00000000 00000000 c003bdf8 fffffffe 00000000 ee2d7f70 ee30da00
[   45.860587] 7f60: 00000000 00000000 dead4ead ffffffff ffffffff ee2d7f74 ee2d7f74 00000000
[   45.868733] 7f80: 00000000 dead4ead ffffffff ffffffff ee2d7f90 ee2d7f90 ee2d7fac eeafefc0
[   45.876878] 7fa0: c003bd20 00000000 00000000 c000f8b8 00000000 00000000 00000000 00000000
[   45.885023] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   45.893169] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ffffffff ffffffff
[   45.901324] [<c0326a00>] (target_submit_cmd_map_sgls) from [<c0326a60>] (target_submit_cmd+0x50/0x58)
[   45.910514] [<c0326a60>] (target_submit_cmd) from [<bf012358>] (bot_cmd_work+0x74/0x100 [usb_f_tcm])
[   45.919623] [<bf012358>] (bot_cmd_work [usb_f_tcm]) from [<c00369bc>] (process_one_work+0x120/0x324)
[   45.928703] [<c00369bc>] (process_one_work) from [<c0036c24>] (worker_thread+0x2c/0x4b4)
[   45.936760] [<c0036c24>] (worker_thread) from [<c003bdf8>] (kthread+0xd8/0xf4)
[   45.943955] [<c003bdf8>] (kthread) from [<c000f8b8>] (ret_from_fork+0x14/0x3c)
[   45.951142] Code: ebf3d01a eaffffd0 ebfbc490 eafffff8 (e7f001f2)
[   45.957206] ---[ end trace 21ce9edfaf719456 ]---
[   45.962132] Unable to handle kernel paging request at virtual address ffffffe0
[   45.968986] pgd = c0004000
[   45.971672] [ffffffe0] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[   45.977913] Internal error: Oops: 37 [#2] PREEMPT SMP ARM
[   45.983282] Modules linked in: usb_f_tcm libcomposite
[   45.988313] CPU: 2 PID: 1221 Comm: kworker/2:1 Tainted: G      D         4.5.0-rc5+ #171
[   45.996367] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   46.002441] task: edc1f6c0 ti: ee2d6000 task.ti: ee2d6000
[   46.007810] PC is at kthread_data+0x4/0xc
[   46.011797] LR is at wq_worker_sleeping+0xc/0xd8
[   46.016389] pc : [<c003c42c>]    lr : [<c0037ac8>]    psr: 00000093
[   46.016389] sp : ee2d7cc8  ip : 00000000  fp : ee2d7cf4
[   46.027824] r10: c07f4ac0  r9 : 00000000  r8 : 00000002
[   46.033024] r7 : c07f85b8  r6 : edc1fa0c  r5 : edc1f6c0  r4 : 00000002
[   46.039523] r3 : 00000000  r2 : 00000000  r1 : 00000002  r0 : edc1f6c0
[   46.046023] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   46.053214] Control: 10c5387d  Table: 6d44806a  DAC: 00000051
[   46.058933] Process kworker/2:1 (pid: 1221, stack limit = 0xee2d6210)
[   46.065345] Stack: (0xee2d7cc8 to 0xee2d8000)
[   46.069683] 7cc0:                   eefa2ac0 c054f544 00000002 ee2d7a2c ee888000 edc1f978
[   46.077829] 7ce0: ee2d7d00 00000000 edc1f6c0 00000001 ee2d7cfc c054f92c edc1f6c0 c0025404
[   46.085975] 7d00: ee2d7d00 ee2d7d00 00000001 c07fc4b4 c0834404 0000000b c06a1620 00000000
[   46.094120] 7d20: edc1f6c0 60000093 c07f8100 c00125d8 ee2d6210 0000000b 07f001f2 c07fc4c8
[   46.102266] 7d40: e7f001f2 ee2d7e00 c0326a00 c0012f20 00000000 c00090bc 00000006 ffff0a10
[   46.110411] 7d60: 00000004 00000000 00030001 c0326a00 00000600 ffff0a10 00000000 c083db90
[   46.118557] 7d80: c08366ac 00000000 c08366ac 00000001 c083dba0 c083db90 00000016 c0222ae8
[   46.126702] 7da0: ffff3a11 ee2d7db0 c00600d0 c06a862c 0000002d 000a513e 00000002 20000093
[   46.134848] 7dc0: c07ffe1c 0000015e 00000000 c08361a0 00000006 c0553e58 00000002 0000015e
[   46.142993] 7de0: 00000000 c0326a04 00000000 c0013358 00000000 e7100000 ee2d6000 c0012f20
[   46.151139] 7e00: ed5939e0 eea71400 ed593988 bf0144d8 ed5939e0 00000000 ee00d830 ed593b8c
[   46.159284] 7e20: ed593988 eefa25c0 00000000 c07f8100 00000000 ee2d7e50 eea71400 c0326a00
[   46.167430] 7e40: a0000013 ffffffff 00000051 c00504a0 002293a6 00000000 00000000 00000000
[   46.175575] 7e60: 00000000 00000000 00000003 00000020 00000000 c0326a60 00000000 00000000
[   46.183720] 7e80: 00000000 00000020 00000003 00000000 00000000 00000000 00000000 00000000
[   46.191866] 7ea0: 00000000 00000000 ed5939cc 00000020 ed5939e0 edf6b580 00000000 bf012358
[   46.200011] 7ec0: 00000000 00000000 00000000 00000020 00000003 00000000 eefa2ac0 ee30da00
[   46.208157] 7ee0: ed5939cc eefa25c0 eefaaa00 c00369bc edc1f6c0 ee30da00 00000001 ee30da00
[   46.216302] 7f00: eefa25e4 00000001 eefa25c0 ee30da18 eefa25c0 00000008 c07f8100 c0036c24
[   46.224448] 7f20: edc1f6c0 eeafefc0 00000000 eeafefc0 00000000 ee30da00 c0036bf8 00000000
[   46.232593] 7f40: 00000000 00000000 00000000 c003bdf8 fffffffe 00000000 ee2d7f70 ee30da00
[   46.240739] 7f60: 00000000 00000000 dead4ead ffffffff ffffffff ee2d7f74 ee2d7f74 00000001
[   46.248885] 7f80: 00010001 dead4ead ffffffff ffffffff ee2d7f90 ee2d7f90 ee2d7fac eeafefc0
[   46.257029] 7fa0: c003bd20 00000000 00000000 c000f8b8 00000000 00000000 00000000 00000000
[   46.265175] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   46.273321] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ffffffff ffffffff
[   46.281472] [<c003c42c>] (kthread_data) from [<c0037ac8>] (wq_worker_sleeping+0xc/0xd8)
[   46.289450] [<c0037ac8>] (wq_worker_sleeping) from [<c054f544>] (__schedule+0x260/0x450)
[   46.297503] [<c054f544>] (__schedule) from [<c054f92c>] (schedule+0x40/0xac)
[   46.304521] [<c054f92c>] (schedule) from [<c0025404>] (do_exit+0x620/0x9c4)
[   46.311454] [<c0025404>] (do_exit) from [<c00125d8>] (die+0x20c/0x2e0)
[   46.317950] [<c00125d8>] (die) from [<c00090bc>] (do_undefinstr+0xb8/0x220)
[   46.324881] [<c00090bc>] (do_undefinstr) from [<c0012f20>] (__und_svc_finish+0x0/0x20)
[   46.332760] Exception stack(0xee2d7e00 to 0xee2d7e48)
[   46.337792] 7e00: ed5939e0 eea71400 ed593988 bf0144d8 ed5939e0 00000000 ee00d830 ed593b8c
[   46.345937] 7e20: ed593988 eefa25c0 00000000 c07f8100 00000000 ee2d7e50 eea71400 c0326a00
[   46.354077] 7e40: a0000013 ffffffff
[   46.357552] [<c0012f20>] (__und_svc_finish) from [<c0326a00>] (target_submit_cmd_map_sgls+0x1fc/0x20c)
[   46.366822] [<c0326a00>] (target_submit_cmd_map_sgls) from [<c0326a60>] (target_submit_cmd+0x50/0x58)
[   46.376023] [<c0326a60>] (target_submit_cmd) from [<bf012358>] (bot_cmd_work+0x74/0x100 [usb_f_tcm])
[   46.385121] [<bf012358>] (bot_cmd_work [usb_f_tcm]) from [<c00369bc>] (process_one_work+0x120/0x324)
[   46.394206] [<c00369bc>] (process_one_work) from [<c0036c24>] (worker_thread+0x2c/0x4b4)
[   46.402264] [<c0036c24>] (worker_thread) from [<c003bdf8>] (kthread+0xd8/0xf4)
[   46.409456] [<c003bdf8>] (kthread) from [<c000f8b8>] (ret_from_fork+0x14/0x3c)
[   46.416647] Code: c003ba54 c06a4010 c0835c50 e5903310 (e5130020)
[   46.422710] ---[ end trace 21ce9edfaf719457 ]---
[   46.427299] Fixing recursive fault but reboot is needed!

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-09 12:53     ` Andrzej Pietrasiewicz
@ 2016-03-10  5:19       ` Nicholas A. Bellinger
  2016-03-10  8:34         ` Andrzej Pietrasiewicz
  0 siblings, 1 reply; 10+ messages in thread
From: Nicholas A. Bellinger @ 2016-03-10  5:19 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz
  Cc: Felipe Balbi, Dan Carpenter, Christoph Hellwig,
	Greg Kroah-Hartman, Sebastian Andrzej Siewior, Bart Van Assche,
	linux-usb, linux-kernel, target-devel

Hi Andrzej,

On Wed, 2016-03-09 at 13:53 +0100, Andrzej Pietrasiewicz wrote:
> Hi Nicholas,
> 
> W dniu 05.03.2016 o 08:26, Nicholas A. Bellinger pisze:
> > Hi Felipe + usb-gadget folks,
> >
> > On Wed, 2016-03-02 at 13:55 +0200, Felipe Balbi wrote:
> 
> <snip>
> 
> >
> > usb-gadget/tcm: Conversion to percpu_ida tag pre-allocation
> > http://www.spinics.net/lists/target-devel/msg11777.html
> >
> > usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs
> > http://www.spinics.net/lists/target-devel/msg11782.html
> >
> > Felipe, Sebastian, & Andrezj, would you be so kind to review and test
> > usb-gadget using target-pending/for-next code..?
> >
> >
> 
> Actually I have noticed a problem at
> 8b00965 "target: Convert demo-mode only drivers to target_alloc_session"
> 
> I get:
> 
> [ 1698.406304] configfs-gadget gadget: high-speed config #1: c
> [ 1698.410547] Using the BOT protocol
> [ 1698.414163] Missing nexus, ignoring command
> [ 1698.417944] bot_cmd_complete(309): -22
> 
> I think the third message is from f_tcm. It is because
> tpg->tpg_nexus is not set anymore, because the line
> 
> 	tpg->tpg_nexus = tv_nexus;
> 
> is removed by the commit mentioned above.
> 
> Restoring this line fixes the problem.

Thanks for testing!

Applying the following patch to re-add the missing assingment
as a proper alloc_session callback.

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index e352a31..348140c 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1570,6 +1570,16 @@ out:
        return ret;
 }
 
+static int usbg_alloc_sess_cb(struct se_portal_group *se_tpg,
+                             struct se_session *se_sess, void *p)
+{
+       struct usbg_tpg *tpg = container_of(se_tpg,
+                               struct usbg_tpg, se_tpg);
+
+       tpg->tpg_nexus = p;
+       return 0;
+}
+
 static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
 {
        struct tcm_usbg_nexus *tv_nexus;
@@ -1591,7 +1601,7 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
        tv_nexus->tvn_se_sess = target_alloc_session(&tpg->se_tpg, 128,
                                                     sizeof(struct usbg_cmd),
                                                     TARGET_PROT_NORMAL, name,
-                                                    tv_nexus, NULL);
+                                                    tv_nexus, usbg_alloc_sess_cb);
        if (IS_ERR(tv_nexus->tvn_se_sess)) {
 #define MAKE_NEXUS_MSG "core_tpg_check_initiator_node_acl() failed for %s\n"
                pr_debug(MAKE_NEXUS_MSG, name);

> 
> Then percpu_ida commit produces the below result
> (it does not happen immediately, but a while after
> running the script).
> I did not bisect, though; I only checked the commits
> which alter drivers/usb/gadget/function/f_tcm.c.
> The last one which (almost) works is:
> 
> 8b00965 "target: Convert demo-mode only drivers to target_alloc_session"
> 
> AP
> 
> I used the following script:
> 
> 8<----------------------------
> 
> #!/bin/bash
> 
> mount -t configfs none /sys/kernel/config
> modprobe libcomposite
> cd /sys/kernel/config/usb_gadget
> mkdir tcm
> cd tcm
> mkdir functions/tcm.0
> cd /sys/kernel/config/target/
> mkdir usb_gadget
> cd usb_gadget
> mkdir naa.0123456789abcdef
> cd naa.0123456789abcdef
> mkdir tpgt_1
> cd tpgt_1
> echo naa.01234567890abcdef > nexus
> echo 1 > enable
> cd /sys/kernel/config/usb_gadget/tcm
> mkdir configs/c.1
> ln -s functions/tcm.0 configs/c.1
> echo 0x0525 > idVendor
> echo 0x1234 > idProduct
> echo 12400000.dwc3 > UDC
> 
> 8<----------------------------
> 
> # [   45.510513] ERR bot_status_complete(73)
> [   45.671921] configfs-gadget gadget: high-speed config #1: c
> [   45.676158] Using the BOT protocol
> [   45.679860] ------------[ cut here ]------------
> [   45.683981] kernel BUG at drivers/target/target_core_transport.c:1476!

Mmmm, usbg_get_cmd() was missing an explicit memset() after tag lookup.

How about the following..?

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index e352a31..d4e8a91 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1078,6 +1078,7 @@ static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
                return ERR_PTR(-ENOMEM);
 
        cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[tag];
+       memset(cmd, 0, sizeof(*cmd));
        cmd->se_cmd.map_tag = tag;
        cmd->se_cmd.tag = cmd->tag = scsi_tag;
        cmd->fu = fu;

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-09 11:38     ` Felipe Balbi
@ 2016-03-10  6:23       ` Nicholas A. Bellinger
  0 siblings, 0 replies; 10+ messages in thread
From: Nicholas A. Bellinger @ 2016-03-10  6:23 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Dan Carpenter, Christoph Hellwig, Greg Kroah-Hartman,
	Sebastian Andrzej Siewior, Andrzej Pietrasiewicz,
	Bart Van Assche, linux-usb, linux-kernel, target-devel

On Wed, 2016-03-09 at 13:38 +0200, Felipe Balbi wrote:
> Hi,
> 
> "Nicholas A. Bellinger" <nab@linux-iscsi.org> writes:
> > [ text/plain ]
> > Hi Felipe + usb-gadget folks,
> >
> > On Wed, 2016-03-02 at 13:55 +0200, Felipe Balbi wrote:
> >> Dan Carpenter <dan.carpenter@oracle.com> writes:
> >> > We need to move the kfree() down a line so we don't dereference a freed
> >> > variable.
> >> >
> >> > Fixes: 1b418a8fcbc0 ('target: Convert demo-mode only drivers to target_alloc_session')
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> 
> >> It's okay to take this via target:
> >> 
> >> Signed-off-by: Felipe Balbi <balbi@kernel.org>
> >> 
> >
> > Note this specific patch is only a mechanical change, and we still need
> > reviews for the more interesting conversions:
> >
> > usb-gadget/tcm: Conversion to percpu_ida tag pre-allocation
> > http://www.spinics.net/lists/target-devel/msg11777.html
> >
> > usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs
> > http://www.spinics.net/lists/target-devel/msg11782.html
> >
> I don't have either patch in my inbox apparently. Care to resend ?
> 
> sorry
> 

A -v4 is going on this week, and will make sure they hit your inbox.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-10  5:19       ` Nicholas A. Bellinger
@ 2016-03-10  8:34         ` Andrzej Pietrasiewicz
  2016-03-11  4:10           ` Nicholas A. Bellinger
  0 siblings, 1 reply; 10+ messages in thread
From: Andrzej Pietrasiewicz @ 2016-03-10  8:34 UTC (permalink / raw)
  To: Nicholas A. Bellinger
  Cc: Felipe Balbi, Dan Carpenter, Christoph Hellwig,
	Greg Kroah-Hartman, Sebastian Andrzej Siewior, Bart Van Assche,
	linux-usb, linux-kernel, target-devel

Hi Nicholas,

W dniu 10.03.2016 o 06:19, Nicholas A. Bellinger pisze:
> Hi Andrzej,
>
> On Wed, 2016-03-09 at 13:53 +0100, Andrzej Pietrasiewicz wrote:
>> Hi Nicholas,
>>

<snip>

>
> Applying the following patch to re-add the missing assingment
> as a proper alloc_session callback.
>
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index e352a31..348140c 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1570,6 +1570,16 @@ out:
>          return ret;
>   }
>
> +static int usbg_alloc_sess_cb(struct se_portal_group *se_tpg,
> +                             struct se_session *se_sess, void *p)
> +{
> +       struct usbg_tpg *tpg = container_of(se_tpg,
> +                               struct usbg_tpg, se_tpg);
> +
> +       tpg->tpg_nexus = p;
> +       return 0;
> +}
> +
>   static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
>   {
>          struct tcm_usbg_nexus *tv_nexus;
> @@ -1591,7 +1601,7 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
>          tv_nexus->tvn_se_sess = target_alloc_session(&tpg->se_tpg, 128,
>                                                       sizeof(struct usbg_cmd),
>                                                       TARGET_PROT_NORMAL, name,
> -                                                    tv_nexus, NULL);
> +                                                    tv_nexus, usbg_alloc_sess_cb);
>          if (IS_ERR(tv_nexus->tvn_se_sess)) {
>   #define MAKE_NEXUS_MSG "core_tpg_check_initiator_node_acl() failed for %s\n"
>                  pr_debug(MAKE_NEXUS_MSG, name);
>

<snip>

>
> Mmmm, usbg_get_cmd() was missing an explicit memset() after tag lookup.
>
> How about the following..?
>
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index e352a31..d4e8a91 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1078,6 +1078,7 @@ static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
>                  return ERR_PTR(-ENOMEM);
>
>          cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[tag];
> +       memset(cmd, 0, sizeof(*cmd));
>          cmd->se_cmd.map_tag = tag;
>          cmd->se_cmd.tag = cmd->tag = scsi_tag;
>          cmd->fu = fu;
>
>
>

I tested it. Works for me.

AP

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch -target tree] usb: gadget: f_tcm: use after free
  2016-03-10  8:34         ` Andrzej Pietrasiewicz
@ 2016-03-11  4:10           ` Nicholas A. Bellinger
  0 siblings, 0 replies; 10+ messages in thread
From: Nicholas A. Bellinger @ 2016-03-11  4:10 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz
  Cc: Felipe Balbi, Dan Carpenter, Christoph Hellwig,
	Greg Kroah-Hartman, Sebastian Andrzej Siewior, Bart Van Assche,
	linux-usb, linux-kernel, target-devel

On Thu, 2016-03-10 at 09:34 +0100, Andrzej Pietrasiewicz wrote:
> Hi Nicholas,
> 
> W dniu 10.03.2016 o 06:19, Nicholas A. Bellinger pisze:
> > Hi Andrzej,
> >
> > On Wed, 2016-03-09 at 13:53 +0100, Andrzej Pietrasiewicz wrote:
> >> Hi Nicholas,
> >>

<SNIP>

> >
> > Mmmm, usbg_get_cmd() was missing an explicit memset() after tag lookup.
> >
> > How about the following..?
> >
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index e352a31..d4e8a91 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -1078,6 +1078,7 @@ static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
> >                  return ERR_PTR(-ENOMEM);
> >
> >          cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[tag];
> > +       memset(cmd, 0, sizeof(*cmd));
> >          cmd->se_cmd.map_tag = tag;
> >          cmd->se_cmd.tag = cmd->tag = scsi_tag;
> >          cmd->fu = fu;
> >
> >
> >
> 
> I tested it. Works for me.

Folding this missing memset() into usb-gadget's percpu_ida conversion
for -v4.

Thanks Andrzej!

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-11  4:10 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-02 10:08 [patch -target tree] usb: gadget: f_tcm: use after free Dan Carpenter
2016-03-02 11:55 ` Felipe Balbi
2016-03-05  7:26   ` Nicholas A. Bellinger
2016-03-09 11:38     ` Felipe Balbi
2016-03-10  6:23       ` Nicholas A. Bellinger
2016-03-09 12:53     ` Andrzej Pietrasiewicz
2016-03-10  5:19       ` Nicholas A. Bellinger
2016-03-10  8:34         ` Andrzej Pietrasiewicz
2016-03-11  4:10           ` Nicholas A. Bellinger
2016-03-05  7:20 ` Nicholas A. Bellinger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.