Re: [PATCH] usb: typec: altmode should keep reference to parent

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Fri, Oct 04, 2024 at 09:37:38AM -0300, Thadeu Lima de Souza Cascardo wrote:
> The altmode device release refers to its parent device, but without keeping
> a reference to it.
> 
> When registering the altmode, get a reference to the parent and put it in
> the release function.
> 
> Before this fix, when using CONFIG_DEBUG_KOBJECT_RELEASE, we see issues
> like this:
> 
> [   43.572860] kobject: 'port0.0' (ffff8880057ba008): kobject_release, parent 0000000000000000 (delayed 3000)
> [   43.573532] kobject: 'port0.1' (ffff8880057bd008): kobject_release, parent 0000000000000000 (delayed 1000)
> [   43.574407] kobject: 'port0' (ffff8880057b9008): kobject_release, parent 0000000000000000 (delayed 3000)
> [   43.575059] kobject: 'port1.0' (ffff8880057ca008): kobject_release, parent 0000000000000000 (delayed 4000)
> [   43.575908] kobject: 'port1.1' (ffff8880057c9008): kobject_release, parent 0000000000000000 (delayed 4000)
> [   43.576908] kobject: 'typec' (ffff8880062dbc00): kobject_release, parent 0000000000000000 (delayed 4000)
> [   43.577769] kobject: 'port1' (ffff8880057bf008): kobject_release, parent 0000000000000000 (delayed 3000)
> [   46.612867] ==================================================================
> [   46.613402] BUG: KASAN: slab-use-after-free in typec_altmode_release+0x38/0x129
> [   46.614003] Read of size 8 at addr ffff8880057b9118 by task kworker/2:1/48
> [   46.614538]
> [   46.614668] CPU: 2 UID: 0 PID: 48 Comm: kworker/2:1 Not tainted 6.12.0-rc1-00138-gedbae730ad31 #535
> [   46.615391] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> [   46.616042] Workqueue: events kobject_delayed_cleanup
> [   46.616446] Call Trace:
> [   46.616648]  <TASK>
> [   46.616820]  dump_stack_lvl+0x5b/0x7c
> [   46.617112]  ? typec_altmode_release+0x38/0x129
> [   46.617470]  print_report+0x14c/0x49e
> [   46.617769]  ? rcu_read_unlock_sched+0x56/0x69
> [   46.618117]  ? __virt_addr_valid+0x19a/0x1ab
> [   46.618456]  ? kmem_cache_debug_flags+0xc/0x1d
> [   46.618807]  ? typec_altmode_release+0x38/0x129
> [   46.619161]  kasan_report+0x8d/0xb4
> [   46.619447]  ? typec_altmode_release+0x38/0x129
> [   46.619809]  ? process_scheduled_works+0x3cb/0x85f
> [   46.620185]  typec_altmode_release+0x38/0x129
> [   46.620537]  ? process_scheduled_works+0x3cb/0x85f
> [   46.620907]  device_release+0xaf/0xf2
> [   46.621206]  kobject_delayed_cleanup+0x13b/0x17a
> [   46.621584]  process_scheduled_works+0x4f6/0x85f
> [   46.621955]  ? __pfx_process_scheduled_works+0x10/0x10
> [   46.622353]  ? hlock_class+0x31/0x9a
> [   46.622647]  ? lock_acquired+0x361/0x3c3
> [   46.622956]  ? move_linked_works+0x46/0x7d
> [   46.623277]  worker_thread+0x1ce/0x291
> [   46.623582]  ? __kthread_parkme+0xc8/0xdf
> [   46.623900]  ? __pfx_worker_thread+0x10/0x10
> [   46.624236]  kthread+0x17e/0x190
> [   46.624501]  ? kthread+0xfb/0x190
> [   46.624756]  ? __pfx_kthread+0x10/0x10
> [   46.625015]  ret_from_fork+0x20/0x40
> [   46.625268]  ? __pfx_kthread+0x10/0x10
> [   46.625532]  ret_from_fork_asm+0x1a/0x30
> [   46.625805]  </TASK>
> [   46.625953]
> [   46.626056] Allocated by task 678:
> [   46.626287]  kasan_save_stack+0x24/0x44
> [   46.626555]  kasan_save_track+0x14/0x2d
> [   46.626811]  __kasan_kmalloc+0x3f/0x4d
> [   46.627049]  __kmalloc_noprof+0x1bf/0x1f0
> [   46.627362]  typec_register_port+0x23/0x491
> [   46.627698]  cros_typec_probe+0x634/0xbb6
> [   46.628026]  platform_probe+0x47/0x8c
> [   46.628311]  really_probe+0x20a/0x47d
> [   46.628605]  device_driver_attach+0x39/0x72
> [   46.628940]  bind_store+0x87/0xd7
> [   46.629213]  kernfs_fop_write_iter+0x1aa/0x218
> [   46.629574]  vfs_write+0x1d6/0x29b
> [   46.629856]  ksys_write+0xcd/0x13b
> [   46.630128]  do_syscall_64+0xd4/0x139
> [   46.630420]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [   46.630820]
> [   46.630946] Freed by task 48:
> [   46.631182]  kasan_save_stack+0x24/0x44
> [   46.631493]  kasan_save_track+0x14/0x2d
> [   46.631799]  kasan_save_free_info+0x3f/0x4d
> [   46.632144]  __kasan_slab_free+0x37/0x45
> [   46.632474]  kfree+0x1d4/0x252
> [   46.632725]  device_release+0xaf/0xf2
> [   46.633017]  kobject_delayed_cleanup+0x13b/0x17a
> [   46.633388]  process_scheduled_works+0x4f6/0x85f
> [   46.633764]  worker_thread+0x1ce/0x291
> [   46.634065]  kthread+0x17e/0x190
> [   46.634324]  ret_from_fork+0x20/0x40
> [   46.634621]  ret_from_fork_asm+0x1a/0x30
> 
> Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

I tried to go through the code and the logs, but I did not really find
any explanation why we couldn't keep the device release order in sync
for all devices that need it. I'll see if we can improve that somehow
in the core, but we can fix this one in the meantime.

But this is a case of use-after-free, so shouldn't this go to the
stable trees?

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

thanks,

-- 
heikki