All of lore.kernel.org
 help / color / mirror / Atom feed
* [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
@ 2020-08-13 15:29 John Garry
  2020-08-13 18:28 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 12+ messages in thread
From: John Garry @ 2020-08-13 15:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-usb

Hi,

I see this on today's linux-next with notably KASAN and 
DEBUG_TEST_DRIVER_REMOVE enabled:

[58.045441] hub 6-0:1.0: 1 port detected
[58.050894] xhci_hcd 0000:7a:02.0: remove, state 1
[58.055698] usb usb6: USB disconnect, device number 1
[58.063151] xhci_hcd 0000:7a:02.0: USB bus 6 deregistered
[58.068598] xhci_hcd 0000:7a:02.0: remove, state 1
[58.073396] usb usb5: USB disconnect, device number 1
[58.082040] xhci_hcd 0000:7a:02.0: USB bus 5 deregistered
[58.088446] 
==================================================================
[58.095667] BUG: KASAN: use-after-free in usb_hcd_pci_remove+0xbc/0x168
[58.102272] Read of size 8 at addr ffff002a0b6e8120 by task swapper/0/1
[58.108875]
[58.110361] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G 
W5.8.0-next-20200813-dirty #31
[58.119396] Hardware name: Huawei D06 /D06, BIOS Hisilicon D06 UEFI RC0 
- V1.16.01 03/15/2019
[58.127910] Call trace:
[58.130351]  dump_backtrace+0x0/0x2d0
[58.134005]  show_stack+0x18/0x28
[58.137313]  dump_stack+0xf4/0x168
[58.140709]  print_address_description.constprop.12+0x6c/0x4ec
[58.146532]  kasan_report+0x130/0x200
[58.150186]  __asan_load8+0x9c/0xd8
[58.153666]  usb_hcd_pci_remove+0xbc/0x168
[58.157755]  xhci_pci_remove+0xb8/0x108
[58.161583]  pci_device_remove+0x6c/0x138
[58.165586]  really_probe+0x22c/0x640
[58.169240]  driver_probe_device+0x78/0xe8
[58.173328]  device_driver_attach+0x9c/0xa8
[58.177504]  __driver_attach+0x74/0x120
[58.181331]  bus_for_each_dev+0xec/0x160
[58.185245]  driver_attach+0x34/0x48
[58.188812]  bus_add_driver+0x1b8/0x2c0
[58.192639]  driver_register+0xc0/0x1e0
[58.196467]  __pci_register_driver+0xb4/0xd0
[58.200729]  xhci_pci_init+0x60/0x70
[58.204297]  do_one_initcall+0xd4/0x264
[58.208125]  kernel_init_freeable+0x270/0x2d8
[58.212475]  kernel_init+0x14/0x124
[58.215955]  ret_from_fork+0x10/0x34
[58.216996] hub 1-1:1.0: USB hub found
[58.219519]
[58.219523] Allocated by task 13:
[58.219528]  kasan_save_stack+0x28/0x58
[58.219538]  __kasan_kmalloc.isra.6+0xcc/0xf0
[58.236219]  kasan_kmalloc+0x10/0x20
[58.239786]  __kmalloc+0x194/0x290
[58.243181]  __usb_create_hcd+0x48/0x3a8
[58.247095]  usb_create_hcd+0x1c/0x28
[58.250748]  usb_hcd_pci_probe+0xa0/0x548
[58.254749]  xhci_pci_probe+0x54/0x360
[58.258488]  local_pci_probe+0x78/0xf8
[58.262230]  work_for_cpu_fn+0x30/0x50
[58.265971]  process_one_work+0x424/0x698
[58.269972]  worker_thread+0x438/0x6d0
[58.273713]  kthread+0x224/0x230
[58.276933]  ret_from_fork+0x10/0x34
[58.280497]
[58.281978] Freed by task 1:
[58.284851]  kasan_save_stack+0x28/0x58
[58.288677]  kasan_set_track+0x28/0x40
[58.292418]  kasan_set_free_info+0x24/0x48
[58.296505]  __kasan_slab_free+0x104/0x188
[58.300593]  kasan_slab_free+0x14/0x20
[58.304333]  slab_free_freelist_hook+0x8c/0x190
[58.308855]  kfree+0x2cc/0x400
[58.311901]  usb_put_hcd+0xc0/0x110
[58.315381]  usb_hcd_pci_remove+0xb4/0x168
[58.317397] hub 1-1:1.0: 4 ports detected
[58.319468]  xhci_pci_remove+0xb8/0x108
[58.319473]  pci_device_remove+0x6c/0x138
[58.319480]  really_probe+0x22c/0x640
[58.334943]  driver_probe_device+0x78/0xe8
[58.339031]  device_driver_attach+0x9c/0xa8
[58.343205]  __driver_attach+0x74/0x120
[58.347033]  bus_for_each_dev+0xec/0x160
[58.350947]  driver_attach+0x34/0x48
[58.354513]  bus_add_driver+0x1b8/0x2c0
[58.358340]  driver_register+0xc0/0x1e0
[58.362168]  __pci_register_driver+0xb4/0xd0
[58.366429]  xhci_pci_init+0x60/0x70
[58.369995]  do_one_initcall+0xd4/0x264
[58.373823]  kernel_init_freeable+0x270/0x2d8
[58.378171]  kernel_init+0x14/0x124
[58.381651]  ret_from_fork+0x10/0x34
[58.385215]
[58.386699] The buggy address belongs to the object at ffff002a0b6e8000
[58.386699]  which belongs to the cache kmalloc-8k of size 8192
[58.399207] The buggy address is located 288 bytes inside of
[58.399207]  8192-byte region [ffff002a0b6e8000, ffff002a0b6ea000)
[58.411019] The buggy address belongs to the page:
[58.415803] page:0000000060c9fcec refcount:1 mapcount:0 
mapping:0000000000000000 index:0x0 pfn:0x2a0b6e8
[58.425272] head:0000000060c9fcec order:3 compound_mapcount:0 
compound_pincount:0
[58.432746] flags: 0x2ffff00000010200(slab|head)
[58.437357] raw: 2ffff00000010200 dead000000000100 dead000000000122 
ffff002a53c0f300
[58.445092] raw: 0000000000000000 0000000080020002 00000001ffffffff 
0000000000000000
[58.452824] page dumped because: kasan: bad access detected
[58.458385]
[58.459867] Memory state around the buggy address:
[58.464649]  ffff002a0b6e8000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb 
fb fb
[58.471862]  ffff002a0b6e8080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
fb fb
[58.479074] >ffff002a0b6e8100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
fb fb
[58.486284]  ^
[58.490545]  ffff002a0b6e8180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
fb fb
[58.497758]  ffff002a0b6e8200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
fb fb
[58.504968] 
==================================================================
[58.512179] Disabling lock debugging due to kernel taint
[58.519143] xhci_hcd 0000:7a:02.0: xHCI Host Controller
[58.524428] xhci_hcd 0000:7a:02.0: new USB bus registered, assigned bus 
number 5
[58.532155] xhci_hcd 0000:7a:02.0: hcc params 0x0220f66d hci version 
0x100 quirks 0x0000000000000010
[58.545434] hub 5-0:1.0: USB hub found
[58.549366] hub 5-0:1.0: 1 port detected
[58.554496] hub 5-0:1.0: USB hub found
[58.558406] hub 5-0:1.0: 1 port detected
[58.565161] hub 5-0:1.0: USB hub found
[58.569080] hub 5-0:1.0: 1 port detected
[58.574234] hub 5-0:1.0: USB hub found
[58.578419] hub 5-0:1.0: 1 port detected
[58.583849] xhci_hcd 0000:7a:02.0: xHCI Host Controller
[58.589112] xhci_hcd 0000:7a:02.0: new USB bus registered, assigned bus 
number 6
[58.596755] xhci_hcd 0000:7a:02.0: Host supports USB 3.0 SuperSpeed
[58.603283] usb usb6: We don't know the algorithms for LPM for this 
host, disabling LPM.
[58.614147] hub 6-0:1.0: USB hub found
[58.618068] hub 6-0:1.0: 1 port detected
[58.623291] hub 6-0:1.0: USB hub found
[58.627197] hub 6-0:1.0: 1 port detected
[58.633993] hub 6-0:1.0: USB hub found
[58.638412] hub 6-0:1.0: 1 port detected
[58.643593] hub 6-0:1.0: USB hub found
[58.647537] hub 6-0:1.0: 1 port detected
[58.654020] xhci_hcd 0000:ba:02.0: xHCI Host Controller
[58.659323] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus 
number 7
[58.667086] xhci_hcd 0000:ba:02.0: hcc params 0x0220f66d hci version 
0x100 quirks 0x0000000000000010
[58.680638] hub 7-0:1.0: USB hub found
[58.684572] hub 7-0:1.0: 1 port detected
[58.689782] hub 7-0:1.0: USB hub found
[58.693700] hub 7-0:1.0: 1 port detected
[58.709559] hub 7-0:1.0: USB hub found
[58.713571] hub 7-0:1.0: 1 port detected
[58.718955] xhci_hcd 0000:ba:02.0: xHCI Host Controller
[58.724567] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus 
number 8
[58.731996] xhci_hcd 0000:ba:02.0: Hos.0: remove, state 1
[58.793746] usb usb8: USB disconnect, device number 1
[58.801364] xhci_hcd 0000:ba:02.0: USB bus 8 deregistered
[58.806815] xhci_hcd 0000:ba:02.0: remove, state 1
[58.811614] usb usb7: USB disconnect, device number 1
[58.820394] xhci_hcd 0000:ba:02.0: USB bus 7 deregistered
[58.826505] xhci_hcd 0000:ba:02.0: xHCI Host Controller
[58.831811] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus 
number 7
[58.839569] xhci_hcd 0000:ba:02.0: hcc params 0x0220f66d hci version 
0x100 quirks 0x0000000000000010
[58.852983] hub 7-0:1.0: USB hub found
[58.856908] hub 7-0:1.0: 1 port detected
[58.862044] hub 7-0:1.0: USB hub found
[58.865959] hub 7-0:1.0: 1 port detected
[58.872577] hub 7-0:1.0: USB hub found
[58.876491] hub 7-0:1.0: 1 port detected
[58.881760] hub 7-0:1.0: USB hub found
[58.885678] hub 7-0:1.0: 1 port detected
[58.891037] xhci_hcd 0000:ba:02.0: xHCI Host Controller
[58.896550] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus 
number 8
[58.903968] xhci_hcd 0000:ba:02.0: Host supports USB 3.0 SuperSpeed
[58.910471] usb usb8: We don't know the algorithms for LPM for this 
host, disabling LPM.
[58.921427] hub 8-0:1.0: USB hub found
[58.925533] hub 8-0:1.0: 1 port detected
[58.930781] hub 8-0:1.0: USB hub found
[58.934695] hub 8-0:1.0: 1 port detected
[58.941649] hub 8-0:1.0: USB hub found
[58.945728] hub 8-0:1.0: 1 port detected
[58.950967] hub 8-0:1.0: USB hub found
[58.954894] hub 8-0:1.0: 1 port detected
[58.961651] usbcore: registered new interface driver usb-storage

Ring any bells? I don't mind debugging the real issue, but good to know 
it's not already solved. I didn't see anything on linux-usb archives, 
apart from this:
https://lore.kernel.org/linux-usb/1566554761464.60146@mentor.com/ which 
doesn't look too familiar.

I couldn't recreate on v5.8, but this triggering this seems a bit 
fragile even on linux-next.

Thanks,
john

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-13 15:29 [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove John Garry
@ 2020-08-13 18:28 ` Greg Kroah-Hartman
  2020-08-14  7:26   ` John Garry
  0 siblings, 1 reply; 12+ messages in thread
From: Greg Kroah-Hartman @ 2020-08-13 18:28 UTC (permalink / raw)
  To: John Garry; +Cc: linux-usb

On Thu, Aug 13, 2020 at 04:29:07PM +0100, John Garry wrote:
> Hi,
> 
> I see this on today's linux-next with notably KASAN and
> DEBUG_TEST_DRIVER_REMOVE enabled:
> 
> [58.045441] hub 6-0:1.0: 1 port detected
> [58.050894] xhci_hcd 0000:7a:02.0: remove, state 1
> [58.055698] usb usb6: USB disconnect, device number 1
> [58.063151] xhci_hcd 0000:7a:02.0: USB bus 6 deregistered
> [58.068598] xhci_hcd 0000:7a:02.0: remove, state 1
> [58.073396] usb usb5: USB disconnect, device number 1
> [58.082040] xhci_hcd 0000:7a:02.0: USB bus 5 deregistered
> [58.088446]
> ==================================================================
> [58.095667] BUG: KASAN: use-after-free in usb_hcd_pci_remove+0xbc/0x168
> [58.102272] Read of size 8 at addr ffff002a0b6e8120 by task swapper/0/1
> [58.108875]
> [58.110361] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G
> W5.8.0-next-20200813-dirty #31
> [58.119396] Hardware name: Huawei D06 /D06, BIOS Hisilicon D06 UEFI RC0 -
> V1.16.01 03/15/2019
> [58.127910] Call trace:
> [58.130351]  dump_backtrace+0x0/0x2d0
> [58.134005]  show_stack+0x18/0x28
> [58.137313]  dump_stack+0xf4/0x168
> [58.140709]  print_address_description.constprop.12+0x6c/0x4ec
> [58.146532]  kasan_report+0x130/0x200
> [58.150186]  __asan_load8+0x9c/0xd8
> [58.153666]  usb_hcd_pci_remove+0xbc/0x168
> [58.157755]  xhci_pci_remove+0xb8/0x108
> [58.161583]  pci_device_remove+0x6c/0x138
> [58.165586]  really_probe+0x22c/0x640
> [58.169240]  driver_probe_device+0x78/0xe8
> [58.173328]  device_driver_attach+0x9c/0xa8
> [58.177504]  __driver_attach+0x74/0x120
> [58.181331]  bus_for_each_dev+0xec/0x160
> [58.185245]  driver_attach+0x34/0x48
> [58.188812]  bus_add_driver+0x1b8/0x2c0
> [58.192639]  driver_register+0xc0/0x1e0
> [58.196467]  __pci_register_driver+0xb4/0xd0
> [58.200729]  xhci_pci_init+0x60/0x70
> [58.204297]  do_one_initcall+0xd4/0x264
> [58.208125]  kernel_init_freeable+0x270/0x2d8
> [58.212475]  kernel_init+0x14/0x124
> [58.215955]  ret_from_fork+0x10/0x34
> [58.216996] hub 1-1:1.0: USB hub found
> [58.219519]
> [58.219523] Allocated by task 13:
> [58.219528]  kasan_save_stack+0x28/0x58
> [58.219538]  __kasan_kmalloc.isra.6+0xcc/0xf0
> [58.236219]  kasan_kmalloc+0x10/0x20
> [58.239786]  __kmalloc+0x194/0x290
> [58.243181]  __usb_create_hcd+0x48/0x3a8
> [58.247095]  usb_create_hcd+0x1c/0x28
> [58.250748]  usb_hcd_pci_probe+0xa0/0x548
> [58.254749]  xhci_pci_probe+0x54/0x360
> [58.258488]  local_pci_probe+0x78/0xf8
> [58.262230]  work_for_cpu_fn+0x30/0x50
> [58.265971]  process_one_work+0x424/0x698
> [58.269972]  worker_thread+0x438/0x6d0
> [58.273713]  kthread+0x224/0x230
> [58.276933]  ret_from_fork+0x10/0x34
> [58.280497]
> [58.281978] Freed by task 1:
> [58.284851]  kasan_save_stack+0x28/0x58
> [58.288677]  kasan_set_track+0x28/0x40
> [58.292418]  kasan_set_free_info+0x24/0x48
> [58.296505]  __kasan_slab_free+0x104/0x188
> [58.300593]  kasan_slab_free+0x14/0x20
> [58.304333]  slab_free_freelist_hook+0x8c/0x190
> [58.308855]  kfree+0x2cc/0x400
> [58.311901]  usb_put_hcd+0xc0/0x110
> [58.315381]  usb_hcd_pci_remove+0xb4/0x168
> [58.317397] hub 1-1:1.0: 4 ports detected
> [58.319468]  xhci_pci_remove+0xb8/0x108
> [58.319473]  pci_device_remove+0x6c/0x138
> [58.319480]  really_probe+0x22c/0x640
> [58.334943]  driver_probe_device+0x78/0xe8
> [58.339031]  device_driver_attach+0x9c/0xa8
> [58.343205]  __driver_attach+0x74/0x120
> [58.347033]  bus_for_each_dev+0xec/0x160
> [58.350947]  driver_attach+0x34/0x48
> [58.354513]  bus_add_driver+0x1b8/0x2c0
> [58.358340]  driver_register+0xc0/0x1e0
> [58.362168]  __pci_register_driver+0xb4/0xd0
> [58.366429]  xhci_pci_init+0x60/0x70
> [58.369995]  do_one_initcall+0xd4/0x264
> [58.373823]  kernel_init_freeable+0x270/0x2d8
> [58.378171]  kernel_init+0x14/0x124
> [58.381651]  ret_from_fork+0x10/0x34
> [58.385215]
> [58.386699] The buggy address belongs to the object at ffff002a0b6e8000
> [58.386699]  which belongs to the cache kmalloc-8k of size 8192
> [58.399207] The buggy address is located 288 bytes inside of
> [58.399207]  8192-byte region [ffff002a0b6e8000, ffff002a0b6ea000)
> [58.411019] The buggy address belongs to the page:
> [58.415803] page:0000000060c9fcec refcount:1 mapcount:0
> mapping:0000000000000000 index:0x0 pfn:0x2a0b6e8
> [58.425272] head:0000000060c9fcec order:3 compound_mapcount:0
> compound_pincount:0
> [58.432746] flags: 0x2ffff00000010200(slab|head)
> [58.437357] raw: 2ffff00000010200 dead000000000100 dead000000000122
> ffff002a53c0f300
> [58.445092] raw: 0000000000000000 0000000080020002 00000001ffffffff
> 0000000000000000
> [58.452824] page dumped because: kasan: bad access detected
> [58.458385]
> [58.459867] Memory state around the buggy address:
> [58.464649]  ffff002a0b6e8000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb
> [58.471862]  ffff002a0b6e8080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb
> [58.479074] >ffff002a0b6e8100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb
> [58.486284]  ^
> [58.490545]  ffff002a0b6e8180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb
> [58.497758]  ffff002a0b6e8200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb
> [58.504968]
> ==================================================================
> [58.512179] Disabling lock debugging due to kernel taint
> [58.519143] xhci_hcd 0000:7a:02.0: xHCI Host Controller
> [58.524428] xhci_hcd 0000:7a:02.0: new USB bus registered, assigned bus
> number 5
> [58.532155] xhci_hcd 0000:7a:02.0: hcc params 0x0220f66d hci version 0x100
> quirks 0x0000000000000010
> [58.545434] hub 5-0:1.0: USB hub found
> [58.549366] hub 5-0:1.0: 1 port detected
> [58.554496] hub 5-0:1.0: USB hub found
> [58.558406] hub 5-0:1.0: 1 port detected
> [58.565161] hub 5-0:1.0: USB hub found
> [58.569080] hub 5-0:1.0: 1 port detected
> [58.574234] hub 5-0:1.0: USB hub found
> [58.578419] hub 5-0:1.0: 1 port detected
> [58.583849] xhci_hcd 0000:7a:02.0: xHCI Host Controller
> [58.589112] xhci_hcd 0000:7a:02.0: new USB bus registered, assigned bus
> number 6
> [58.596755] xhci_hcd 0000:7a:02.0: Host supports USB 3.0 SuperSpeed
> [58.603283] usb usb6: We don't know the algorithms for LPM for this host,
> disabling LPM.
> [58.614147] hub 6-0:1.0: USB hub found
> [58.618068] hub 6-0:1.0: 1 port detected
> [58.623291] hub 6-0:1.0: USB hub found
> [58.627197] hub 6-0:1.0: 1 port detected
> [58.633993] hub 6-0:1.0: USB hub found
> [58.638412] hub 6-0:1.0: 1 port detected
> [58.643593] hub 6-0:1.0: USB hub found
> [58.647537] hub 6-0:1.0: 1 port detected
> [58.654020] xhci_hcd 0000:ba:02.0: xHCI Host Controller
> [58.659323] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus
> number 7
> [58.667086] xhci_hcd 0000:ba:02.0: hcc params 0x0220f66d hci version 0x100
> quirks 0x0000000000000010
> [58.680638] hub 7-0:1.0: USB hub found
> [58.684572] hub 7-0:1.0: 1 port detected
> [58.689782] hub 7-0:1.0: USB hub found
> [58.693700] hub 7-0:1.0: 1 port detected
> [58.709559] hub 7-0:1.0: USB hub found
> [58.713571] hub 7-0:1.0: 1 port detected
> [58.718955] xhci_hcd 0000:ba:02.0: xHCI Host Controller
> [58.724567] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus
> number 8
> [58.731996] xhci_hcd 0000:ba:02.0: Hos.0: remove, state 1
> [58.793746] usb usb8: USB disconnect, device number 1
> [58.801364] xhci_hcd 0000:ba:02.0: USB bus 8 deregistered
> [58.806815] xhci_hcd 0000:ba:02.0: remove, state 1
> [58.811614] usb usb7: USB disconnect, device number 1
> [58.820394] xhci_hcd 0000:ba:02.0: USB bus 7 deregistered
> [58.826505] xhci_hcd 0000:ba:02.0: xHCI Host Controller
> [58.831811] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus
> number 7
> [58.839569] xhci_hcd 0000:ba:02.0: hcc params 0x0220f66d hci version 0x100
> quirks 0x0000000000000010
> [58.852983] hub 7-0:1.0: USB hub found
> [58.856908] hub 7-0:1.0: 1 port detected
> [58.862044] hub 7-0:1.0: USB hub found
> [58.865959] hub 7-0:1.0: 1 port detected
> [58.872577] hub 7-0:1.0: USB hub found
> [58.876491] hub 7-0:1.0: 1 port detected
> [58.881760] hub 7-0:1.0: USB hub found
> [58.885678] hub 7-0:1.0: 1 port detected
> [58.891037] xhci_hcd 0000:ba:02.0: xHCI Host Controller
> [58.896550] xhci_hcd 0000:ba:02.0: new USB bus registered, assigned bus
> number 8
> [58.903968] xhci_hcd 0000:ba:02.0: Host supports USB 3.0 SuperSpeed
> [58.910471] usb usb8: We don't know the algorithms for LPM for this host,
> disabling LPM.
> [58.921427] hub 8-0:1.0: USB hub found
> [58.925533] hub 8-0:1.0: 1 port detected
> [58.930781] hub 8-0:1.0: USB hub found
> [58.934695] hub 8-0:1.0: 1 port detected
> [58.941649] hub 8-0:1.0: USB hub found
> [58.945728] hub 8-0:1.0: 1 port detected
> [58.950967] hub 8-0:1.0: USB hub found
> [58.954894] hub 8-0:1.0: 1 port detected
> [58.961651] usbcore: registered new interface driver usb-storage
> 
> Ring any bells? I don't mind debugging the real issue, but good to know it's
> not already solved. I didn't see anything on linux-usb archives, apart from
> this:
> https://lore.kernel.org/linux-usb/1566554761464.60146@mentor.com/ which
> doesn't look too familiar.
> 
> I couldn't recreate on v5.8, but this triggering this seems a bit fragile
> even on linux-next.

Looks new.  Any chance you can run 'git bisect' to track this down?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-13 18:28 ` Greg Kroah-Hartman
@ 2020-08-14  7:26   ` John Garry
  2020-08-14 17:18     ` John Garry
  0 siblings, 1 reply; 12+ messages in thread
From: John Garry @ 2020-08-14  7:26 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb

On 13/08/2020 19:28, Greg Kroah-Hartman wrote:
>> [58.950967] hub 8-0:1.0: USB hub found
>> [58.954894] hub 8-0:1.0: 1 port detected
>> [58.961651] usbcore: registered new interface driver usb-storage
>>
>> Ring any bells? I don't mind debugging the real issue, but good to know it's
>> not already solved. I didn't see anything on linux-usb archives, apart from
>> this:
>> https://lore.kernel.org/linux-usb/1566554761464.60146@mentor.com/  which
>> doesn't look too familiar.
>>
>> I couldn't recreate on v5.8, but this triggering this seems a bit fragile
>> even on linux-next.
> Looks new.  Any chance you can run 'git bisect' to track this down?
> 

Hi Greg,

I need to find a reliable reproducer. I could only reproduce when some 
other seemingly unrelated kernel config options were enabled. Let me see 
if I can reproduce based on Linus' kernel (it will be 5.9-rc now), and 
go from there.

Thanks,
John

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-14  7:26   ` John Garry
@ 2020-08-14 17:18     ` John Garry
  2020-08-14 18:07       ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: John Garry @ 2020-08-14 17:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-usb, Andy Shevchenko

On 14/08/2020 08:26, John Garry wrote:

+

> On 13/08/2020 19:28, Greg Kroah-Hartman wrote:
>>> [58.950967] hub 8-0:1.0: USB hub found
>>> [58.954894] hub 8-0:1.0: 1 port detected
>>> [58.961651] usbcore: registered new interface driver usb-storage
>>>
>>> Ring any bells? I don't mind debugging the real issue, but good to 
>>> know it's
>>> not already solved. I didn't see anything on linux-usb archives, 
>>> apart from
>>> this:
>>> https://lore.kernel.org/linux-usb/1566554761464.60146@mentor.com/  which
>>> doesn't look too familiar.
>>>
>>> I couldn't recreate on v5.8, but this triggering this seems a bit 
>>> fragile
>>> even on linux-next.
>> Looks new.  Any chance you can run 'git bisect' to track this down?
>>
> 
> Hi Greg,
> 
> I need to find a reliable reproducer. I could only reproduce when some 
> other seemingly unrelated kernel config options were enabled. Let me see 
> if I can reproduce based on Linus' kernel (it will be 5.9-rc now), and 
> go from there.
> 

Is the problem 306c54d0edb6 (""usb: hcd: Try MSI interrupts on PCI 
devices), as this following patch stops KASAN complaining for me:

 From a3b9e1b405d1efdfee2ee672bbf7dc1c7de62d66 Mon Sep 17 00:00:00 2001
From: John Garry <john.garry@huawei.com>
Date: Fri, 14 Aug 2020 18:14:51 +0100
Subject: [PATCH] usb: hcd: Fix use-after-free in usb_hcd_pci_remove()

KASAN reports when CONFIG_DEBUG_TEST_DRIVER_REMOVE is enabled:

==================================================================
[58.095667] BUG: KASAN: use-after-free in usb_hcd_pci_remove+0xbc/0x168
[58.102272] Read of size 8 at addr ffff002a0b6e8120 by task swapper/0/1
[58.108875]
[58.110361] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G
W5.8.0-next-20200813-dirty #31
[58.119396] Hardware name: Huawei D06 /D06, BIOS Hisilicon D06 UEFI RC0
- V1.16.01 03/15/2019
[58.127910] Call trace:
[58.130351]  dump_backtrace+0x0/0x2d0
[58.134005]  show_stack+0x18/0x28
[58.137313]  dump_stack+0xf4/0x168
[58.140709]  print_address_description.constprop.12+0x6c/0x4ec
[58.146532]  kasan_report+0x130/0x200
[58.150186]  __asan_load8+0x9c/0xd8
[58.153666]  usb_hcd_pci_remove+0xbc/0x168
[58.157755]  xhci_pci_remove+0xb8/0x108
[58.161583]  pci_device_remove+0x6c/0x138
[58.165586]  really_probe+0x22c/0x640
[58.169240]  driver_probe_device+0x78/0xe8
[58.173328]  device_driver_attach+0x9c/0xa8
[58.177504]  __driver_attach+0x74/0x120
[58.181331]  bus_for_each_dev+0xec/0x160
[58.185245]  driver_attach+0x34/0x48
[58.188812]  bus_add_driver+0x1b8/0x2c0
[58.192639]  driver_register+0xc0/0x1e0
[58.196467]  __pci_register_driver+0xb4/0xd0
[58.200729]  xhci_pci_init+0x60/0x70
[58.204297]  do_one_initcall+0xd4/0x264
[58.208125]  kernel_init_freeable+0x270/0x2d8
[58.212475]  kernel_init+0x14/0x124
[58.215955]  ret_from_fork+0x10/0x34
[58.216996] hub 1-1:1.0: USB hub found
[58.219519]
[58.219523] Allocated by task 13:
[58.219528]  kasan_save_stack+0x28/0x58
[58.219538]  __kasan_kmalloc.isra.6+0xcc/0xf0
[58.236219]  kasan_kmalloc+0x10/0x20
[58.239786]  __kmalloc+0x194/0x290
[58.243181]  __usb_create_hcd+0x48/0x3a8
[58.247095]  usb_create_hcd+0x1c/0x28
[58.250748]  usb_hcd_pci_probe+0xa0/0x548
[58.254749]  xhci_pci_probe+0x54/0x360
[58.258488]  local_pci_probe+0x78/0xf8
[58.262230]  work_for_cpu_fn+0x30/0x50
[58.265971]  process_one_work+0x424/0x698
[58.269972]  worker_thread+0x438/0x6d0
[58.273713]  kthread+0x224/0x230
[58.276933]  ret_from_fork+0x10/0x34
[58.280497]
[58.281978] Freed by task 1:
[58.284851]  kasan_save_stack+0x28/0x58
[58.288677]  kasan_set_track+0x28/0x40
[58.292418]  kasan_set_free_info+0x24/0x48
[58.296505]  __kasan_slab_free+0x104/0x188
[58.300593]  kasan_slab_free+0x14/0x20
[58.304333]  slab_free_freelist_hook+0x8c/0x190
[58.308855]  kfree+0x2cc/0x400
[58.311901]  usb_put_hcd+0xc0/0x110
[58.315381]  usb_hcd_pci_remove+0xb4/0x168
[58.317397] hub 1-1:1.0: 4 ports detected
[58.319468]  xhci_pci_remove+0xb8/0x108
[58.319473]  pci_device_remove+0x6c/0x138
[58.319480]  really_probe+0x22c/0x640
[58.334943]  driver_probe_device+0x78/0xe8
[58.339031]  device_driver_attach+0x9c/0xa8
[58.343205]  __driver_attach+0x74/0x120
[58.347033]  bus_for_each_dev+0xec/0x160
[58.350947]  driver_attach+0x34/0x48
[58.354513]  bus_add_driver+0x1b8/0x2c0
[58.358340]  driver_register+0xc0/0x1e0
[58.362168]  __pci_register_driver+0xb4/0xd0
[58.366429]  xhci_pci_init+0x60/0x70
[58.369995]  do_one_initcall+0xd4/0x264
[58.373823]  kernel_init_freeable+0x270/0x2d8
[58.378171]  kernel_init+0x14/0x124
[58.381651]  ret_from_fork+0x10/0x34
[58.385215]
[58.386699] The buggy address belongs to the object at ffff002a0b6e8000
[58.386699]  which belongs to the cache kmalloc-8k of size 8192
[58.399207] The buggy address is located 288 bytes inside of
[58.399207]  8192-byte region [ffff002a0b6e8000, ffff002a0b6ea000)
[58.411019] The buggy address belongs to the page:
[58.415803] page:0000000060c9fcec refcount:1 mapcount:0
mapping:0000000000000000 index:0x0 pfn:0x2a0b6e8
[58.425272] head:0000000060c9fcec order:3 compound_mapcount:0
compound_pincount:0
[58.432746] flags: 0x2ffff00000010200(slab|head)
[58.437357] raw: 2ffff00000010200 dead000000000100 dead000000000122
ffff002a53c0f300
[58.445092] raw: 0000000000000000 0000000080020002 00000001ffffffff
0000000000000000
[58.452824] page dumped because: kasan: bad access detected
[58.458385]
[58.459867] Memory state around the buggy address:
[58.464649]  ffff002a0b6e8000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb
[58.471862]  ffff002a0b6e8080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb
[58.479074] >ffff002a0b6e8100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb
[58.486284]  ^
[58.490545]  ffff002a0b6e8180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb
[58.497758]  ffff002a0b6e8200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb
[58.504968]
==================================================================

Fix by relocating the usb_put_hcd() call until after the 
hcd->driver->flags check.

Signed-off-by: John Garry <john.garry@huawei.com>

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 4dc443aaef5c..44a8d3644973 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -346,9 +346,9 @@ void usb_hcd_pci_remove(struct pci_dev *dev)
  		dev_set_drvdata(&dev->dev, NULL);
  		up_read(&companions_rwsem);
  	}
-	usb_put_hcd(hcd);
  	if ((hcd->driver->flags & HCD_MASK) < HCD_USB3)
  		pci_free_irq_vectors(dev);
+	usb_put_hcd(hcd);
  	pci_disable_device(dev);
  }
  EXPORT_SYMBOL_GPL(usb_hcd_pci_remove);


---->8-----------

@Andy, Apologies if I'm off the mark!

Thanks,
John


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-14 17:18     ` John Garry
@ 2020-08-14 18:07       ` Andy Shevchenko
  2020-08-14 19:51         ` Alan Stern
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-08-14 18:07 UTC (permalink / raw)
  To: John Garry; +Cc: Greg Kroah-Hartman, linux-usb

On Fri, Aug 14, 2020 at 06:18:16PM +0100, John Garry wrote:

Thanks for notifying me, my comments below.

...

> From a3b9e1b405d1efdfee2ee672bbf7dc1c7de62d66 Mon Sep 17 00:00:00 2001
> From: John Garry <john.garry@huawei.com>
> Date: Fri, 14 Aug 2020 18:14:51 +0100
> Subject: [PATCH] usb: hcd: Fix use-after-free in usb_hcd_pci_remove()
> 
> KASAN reports when CONFIG_DEBUG_TEST_DRIVER_REMOVE is enabled:

Please, reduce these huge trace backs, they have a lot of unneeded bulk.

> ==================================================================
> [58.095667] BUG: KASAN: use-after-free in usb_hcd_pci_remove+0xbc/0x168
> [58.102272] Read of size 8 at addr ffff002a0b6e8120 by task swapper/0/1

...

> [58.497758]  ffff002a0b6e8200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb
> [58.504968]
> ==================================================================
> 
> Fix by relocating the usb_put_hcd() call until after the hcd->driver->flags
> check.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> 
> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> index 4dc443aaef5c..44a8d3644973 100644
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
> @@ -346,9 +346,9 @@ void usb_hcd_pci_remove(struct pci_dev *dev)
>  		dev_set_drvdata(&dev->dev, NULL);
>  		up_read(&companions_rwsem);
>  	}
> -	usb_put_hcd(hcd);
>  	if ((hcd->driver->flags & HCD_MASK) < HCD_USB3)
>  		pci_free_irq_vectors(dev);
> +	usb_put_hcd(hcd);

It's not correct approach.
We need to copy flags to a temporary variable.
I will send a new patch soon to test, thanks!

>  	pci_disable_device(dev);
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_pci_remove);

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-14 18:07       ` Andy Shevchenko
@ 2020-08-14 19:51         ` Alan Stern
       [not found]           ` <CAHp75VdMXd3LWLM5ooBsWGZnSXnJBW3R5gH9Cpux0EHmcxjTvQ@mail.gmail.com>
  0 siblings, 1 reply; 12+ messages in thread
From: Alan Stern @ 2020-08-14 19:51 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: John Garry, Greg Kroah-Hartman, linux-usb

On Fri, Aug 14, 2020 at 09:07:20PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 14, 2020 at 06:18:16PM +0100, John Garry wrote:

> > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> > index 4dc443aaef5c..44a8d3644973 100644
> > --- a/drivers/usb/core/hcd-pci.c
> > +++ b/drivers/usb/core/hcd-pci.c
> > @@ -346,9 +346,9 @@ void usb_hcd_pci_remove(struct pci_dev *dev)
> >  		dev_set_drvdata(&dev->dev, NULL);
> >  		up_read(&companions_rwsem);
> >  	}
> > -	usb_put_hcd(hcd);
> >  	if ((hcd->driver->flags & HCD_MASK) < HCD_USB3)
> >  		pci_free_irq_vectors(dev);
> > +	usb_put_hcd(hcd);
> 
> It's not correct approach.
> We need to copy flags to a temporary variable.
> I will send a new patch soon to test, thanks!

Just out of curiosity, can you explain what is wrong with John's 
approach?  The problem isn't obvious to me.

Alan Stern

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
       [not found]           ` <CAHp75VdMXd3LWLM5ooBsWGZnSXnJBW3R5gH9Cpux0EHmcxjTvQ@mail.gmail.com>
@ 2020-08-15  1:50             ` Alan Stern
  2020-08-16  8:33               ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Alan Stern @ 2020-08-15  1:50 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, John Garry, Greg Kroah-Hartman, linux-usb

On Sat, Aug 15, 2020 at 12:55:57AM +0300, Andy Shevchenko wrote:
> On Friday, August 14, 2020, Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Fri, Aug 14, 2020 at 09:07:20PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 14, 2020 at 06:18:16PM +0100, John Garry wrote:
> >
> > > > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> > > > index 4dc443aaef5c..44a8d3644973 100644
> > > > --- a/drivers/usb/core/hcd-pci.c
> > > > +++ b/drivers/usb/core/hcd-pci.c
> > > > @@ -346,9 +346,9 @@ void usb_hcd_pci_remove(struct pci_dev *dev)
> > > >             dev_set_drvdata(&dev->dev, NULL);
> > > >             up_read(&companions_rwsem);
> > > >     }
> > > > -   usb_put_hcd(hcd);
> > > >     if ((hcd->driver->flags & HCD_MASK) < HCD_USB3)
> > > >             pci_free_irq_vectors(dev);
> > > > +   usb_put_hcd(hcd);
> > >
> > > It's not correct approach.
> > > We need to copy flags to a temporary variable.
> > > I will send a new patch soon to test, thanks!
> >
> > Just out of curiosity, can you explain what is wrong with John's
> > approach?  The problem isn't obvious to me.
> 
> 
> Alloc vector -> create HCD -> put HCD -> free vector
> 
> VS.
> 
> Alloc vector -> create HCD -> free vector -> put HCD
> 
> Of course I might miss something...

Sure, the difference in ordering was pretty obvious.  What is not 
obvious is why this should cause a problem.

Do you think that the host controller driver is going to try to use the 
IRQ vector somewhere between the pci_free_irq_vectors call and the 
usb_put_hcd call?  If that's not going to happen then I don't see what 
difference the order of the two calls makes.

Alan Stern

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-15  1:50             ` Alan Stern
@ 2020-08-16  8:33               ` Andy Shevchenko
  2020-08-16 16:05                 ` Alan Stern
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-08-16  8:33 UTC (permalink / raw)
  To: Alan Stern; +Cc: Andy Shevchenko, John Garry, Greg Kroah-Hartman, linux-usb

On Sat, Aug 15, 2020 at 4:50 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, Aug 15, 2020 at 12:55:57AM +0300, Andy Shevchenko wrote:
> > On Friday, August 14, 2020, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Fri, Aug 14, 2020 at 09:07:20PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Aug 14, 2020 at 06:18:16PM +0100, John Garry wrote:

...

> > > > > -   usb_put_hcd(hcd);
> > > > >     if ((hcd->driver->flags & HCD_MASK) < HCD_USB3)
> > > > >             pci_free_irq_vectors(dev);
> > > > > +   usb_put_hcd(hcd);
> > > >
> > > > It's not correct approach.
> > > > We need to copy flags to a temporary variable.
> > > > I will send a new patch soon to test, thanks!
> > >
> > > Just out of curiosity, can you explain what is wrong with John's
> > > approach?  The problem isn't obvious to me.
> >
> >
> > Alloc vector -> create HCD -> put HCD -> free vector
> >
> > VS.
> >
> > Alloc vector -> create HCD -> free vector -> put HCD
> >
> > Of course I might miss something...
>
> Sure, the difference in ordering was pretty obvious.  What is not
> obvious is why this should cause a problem.

It may be not causing any problem right now, but with all these small
steps we may come to the case like DWC3 removal mess.

> Do you think that the host controller driver is going to try to use the
> IRQ vector somewhere between the pci_free_irq_vectors call and the
> usb_put_hcd call?  If that's not going to happen then I don't see what
> difference the order of the two calls makes.

I think that this is a bit incorrect to rely on side-effects to ruin
the clear understanding of what ordering is going on. If you insist,
you can take John's solution, but I won't give a tag on such.

Also take into consideration the possible copy'n'paste of this example
to other drivers. I have seen a lot of bad examples were
copied'n'pasted all over the kernel during my reviews. I don't want to
give another one.

So, the review process, in my opinion, should be slightly broader that
we usually understand it, i.e. take into account:
- *run-time* bisectability
- possible copy'n'paste of the code excerpts


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-16  8:33               ` Andy Shevchenko
@ 2020-08-16 16:05                 ` Alan Stern
  2020-08-17 11:35                   ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Alan Stern @ 2020-08-16 16:05 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, John Garry, Greg Kroah-Hartman, linux-usb

On Sun, Aug 16, 2020 at 11:33:14AM +0300, Andy Shevchenko wrote:
> On Sat, Aug 15, 2020 at 4:50 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sat, Aug 15, 2020 at 12:55:57AM +0300, Andy Shevchenko wrote:
> > > On Friday, August 14, 2020, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Fri, Aug 14, 2020 at 09:07:20PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Aug 14, 2020 at 06:18:16PM +0100, John Garry wrote:
> 
> ...
> 
> > > > > > -   usb_put_hcd(hcd);
> > > > > >     if ((hcd->driver->flags & HCD_MASK) < HCD_USB3)
> > > > > >             pci_free_irq_vectors(dev);
> > > > > > +   usb_put_hcd(hcd);
> > > > >
> > > > > It's not correct approach.
> > > > > We need to copy flags to a temporary variable.
> > > > > I will send a new patch soon to test, thanks!
> > > >
> > > > Just out of curiosity, can you explain what is wrong with John's
> > > > approach?  The problem isn't obvious to me.
> > >
> > >
> > > Alloc vector -> create HCD -> put HCD -> free vector
> > >
> > > VS.
> > >
> > > Alloc vector -> create HCD -> free vector -> put HCD
> > >
> > > Of course I might miss something...
> >
> > Sure, the difference in ordering was pretty obvious.  What is not
> > obvious is why this should cause a problem.
> 
> It may be not causing any problem right now, but with all these small
> steps we may come to the case like DWC3 removal mess.
> 
> > Do you think that the host controller driver is going to try to use the
> > IRQ vector somewhere between the pci_free_irq_vectors call and the
> > usb_put_hcd call?  If that's not going to happen then I don't see what
> > difference the order of the two calls makes.
> 
> I think that this is a bit incorrect to rely on side-effects to ruin
> the clear understanding of what ordering is going on. If you insist,
> you can take John's solution, but I won't give a tag on such.
> 
> Also take into consideration the possible copy'n'paste of this example
> to other drivers. I have seen a lot of bad examples were
> copied'n'pasted all over the kernel during my reviews. I don't want to
> give another one.
> 
> So, the review process, in my opinion, should be slightly broader that
> we usually understand it, i.e. take into account:
> - *run-time* bisectability
> - possible copy'n'paste of the code excerpts

I see.  So you use "correct" in the broader sense of "good form" as well 
as strict correctness.  (It was confusing because I wouldn't conflate 
those two different concepts.)

Okay, now your reply makes sense.

Alan Stern

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-16 16:05                 ` Alan Stern
@ 2020-08-17 11:35                   ` Andy Shevchenko
  2020-08-18  8:41                     ` John Garry
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-08-17 11:35 UTC (permalink / raw)
  To: Alan Stern; +Cc: John Garry, Greg Kroah-Hartman, linux-usb

On Sun, Aug 16, 2020 at 12:05:50PM -0400, Alan Stern wrote:
> On Sun, Aug 16, 2020 at 11:33:14AM +0300, Andy Shevchenko wrote:
> > On Sat, Aug 15, 2020 at 4:50 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Sat, Aug 15, 2020 at 12:55:57AM +0300, Andy Shevchenko wrote:

...

> > > Sure, the difference in ordering was pretty obvious.  What is not
> > > obvious is why this should cause a problem.
> > 
> > It may be not causing any problem right now, but with all these small
> > steps we may come to the case like DWC3 removal mess.
> > 
> > > Do you think that the host controller driver is going to try to use the
> > > IRQ vector somewhere between the pci_free_irq_vectors call and the
> > > usb_put_hcd call?  If that's not going to happen then I don't see what
> > > difference the order of the two calls makes.
> > 
> > I think that this is a bit incorrect to rely on side-effects to ruin
> > the clear understanding of what ordering is going on. If you insist,
> > you can take John's solution, but I won't give a tag on such.
> > 
> > Also take into consideration the possible copy'n'paste of this example
> > to other drivers. I have seen a lot of bad examples were
> > copied'n'pasted all over the kernel during my reviews. I don't want to
> > give another one.
> > 
> > So, the review process, in my opinion, should be slightly broader that
> > we usually understand it, i.e. take into account:
> > - *run-time* bisectability
> > - possible copy'n'paste of the code excerpts
> 
> I see.  So you use "correct" in the broader sense of "good form" as well 
> as strict correctness.  (It was confusing because I wouldn't conflate 
> those two different concepts.)

Thank you for clarification, I'm not native speaker and this is a good learn
to me. I will try to use better wording in the future.

> Okay, now your reply makes sense.

Thanks!

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-17 11:35                   ` Andy Shevchenko
@ 2020-08-18  8:41                     ` John Garry
  2020-08-18  9:43                       ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: John Garry @ 2020-08-18  8:41 UTC (permalink / raw)
  To: Andy Shevchenko, Alan Stern; +Cc: Greg Kroah-Hartman, linux-usb

On 17/08/2020 12:35, Andy Shevchenko wrote:
> On Sun, Aug 16, 2020 at 12:05:50PM -0400, Alan Stern wrote:
>> On Sun, Aug 16, 2020 at 11:33:14AM +0300, Andy Shevchenko wrote:
>>> On Sat, Aug 15, 2020 at 4:50 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>>>> On Sat, Aug 15, 2020 at 12:55:57AM +0300, Andy Shevchenko wrote:
> 
> ...
> 
>>>> Sure, the difference in ordering was pretty obvious.  What is not
>>>> obvious is why this should cause a problem.
>>>
>>> It may be not causing any problem right now, but with all these small
>>> steps we may come to the case like DWC3 removal mess.
>>>
>>>> Do you think that the host controller driver is going to try to use the
>>>> IRQ vector somewhere between the pci_free_irq_vectors call and the
>>>> usb_put_hcd call?  If that's not going to happen then I don't see what
>>>> difference the order of the two calls makes.
>>>
>>> I think that this is a bit incorrect to rely on side-effects to ruin
>>> the clear understanding of what ordering is going on. If you insist,
>>> you can take John's solution, but I won't give a tag on such.
>>>
>>> Also take into consideration the possible copy'n'paste of this example
>>> to other drivers. I have seen a lot of bad examples were
>>> copied'n'pasted all over the kernel during my reviews. I don't want to
>>> give another one.
>>>
>>> So, the review process, in my opinion, should be slightly broader that
>>> we usually understand it, i.e. take into account:
>>> - *run-time* bisectability
>>> - possible copy'n'paste of the code excerpts
>>
>> I see.  So you use "correct" in the broader sense of "good form" as well
>> as strict correctness.  (It was confusing because I wouldn't conflate
>> those two different concepts.)
> 
> Thank you for clarification, I'm not native speaker and this is a good learn
> to me. I will try to use better wording in the future.
> 
>> Okay, now your reply makes sense.
> 
> Thanks!
> 

It looks like that function [pci_free_irq_vectors()] is harmless when 
the vectors aren't allocated, so it should be possible to always call it 
always and drop the hcd flags check. But then this pattern may not be 
liked either.

Anyway, I guess you guys can sort this out. I'm just trying to help 
identify issues.

Thanks,
John

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove
  2020-08-18  8:41                     ` John Garry
@ 2020-08-18  9:43                       ` Andy Shevchenko
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-08-18  9:43 UTC (permalink / raw)
  To: John Garry; +Cc: Alan Stern, Greg Kroah-Hartman, linux-usb

On Tue, Aug 18, 2020 at 11:44 AM John Garry <john.garry@huawei.com> wrote:
> On 17/08/2020 12:35, Andy Shevchenko wrote:

...

> It looks like that function [pci_free_irq_vectors()] is harmless when
> the vectors aren't allocated, so it should be possible to always call it
> always and drop the hcd flags check. But then this pattern may not be
> liked either.

Yeah.

> Anyway, I guess you guys can sort this out. I'm just trying to help
> identify issues.

Thanks for the report, can you test [1] if it fixes the problem?

[1]: https://lore.kernel.org/linux-usb/20200814182218.71957-1-andriy.shevchenko@linux.intel.com/T/#u

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-08-18  9:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-13 15:29 [Report]: BUG: KASAN: use-after-free in usb_hcd_pci_remove John Garry
2020-08-13 18:28 ` Greg Kroah-Hartman
2020-08-14  7:26   ` John Garry
2020-08-14 17:18     ` John Garry
2020-08-14 18:07       ` Andy Shevchenko
2020-08-14 19:51         ` Alan Stern
     [not found]           ` <CAHp75VdMXd3LWLM5ooBsWGZnSXnJBW3R5gH9Cpux0EHmcxjTvQ@mail.gmail.com>
2020-08-15  1:50             ` Alan Stern
2020-08-16  8:33               ` Andy Shevchenko
2020-08-16 16:05                 ` Alan Stern
2020-08-17 11:35                   ` Andy Shevchenko
2020-08-18  8:41                     ` John Garry
2020-08-18  9:43                       ` Andy Shevchenko

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.