linux-kernel - Re: [PATCH] xhci: Do not create endpoint debugfs while holding the bandwidth mutex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e153b3e8-6c0a-a142-c357-eb295eecdece@linux.intel.com>
Date:   Mon, 12 Jun 2023 17:30:05 +0300
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Ricardo Ribalda <ribalda@...omium.org>
Cc:     Mathias Nyman <mathias.nyman@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Stephen Boyd <swboyd@...omium.org>
Subject: Re: [PATCH] xhci: Do not create endpoint debugfs while holding the
 bandwidth mutex

On 1.6.2023 19.05, Ricardo Ribalda wrote:
> Hi Mathias
> 
> On Thu, 1 Jun 2023 at 16:13, Mathias Nyman
> <mathias.nyman@...ux.intel.com> wrote:
>>
>> Do you still have the lockdep output showing the deadlock?
> 
> [  459.731142] ======================================================
> [  459.731150] WARNING: possible circular locking dependency detected
> [  459.731161] 5.4.169-lockdep-17434-g505c8a10e6fe #1 Not tainted
> [  459.731168] ------------------------------------------------------
> [  459.731176] syz-executor.3/15308 is trying to acquire lock:
> [  459.731184] ffffff80c63e0ee0 (&queue->mutex){+.+.}, at:
> uvc_queue_mmap+0x30/0xa0 [uvcvideo]
> [  459.731226]
>                 but task is already holding lock:
> [  459.731232] ffffff80a748eea8 (&mm->mmap_sem){++++}, at:
> vm_mmap_pgoff+0x10c/0x1f4
> [  459.731255]
>                 which lock already depends on the new lock.
> 
...
> [  459.732148] Chain exists of:
>                   &queue->mutex --> &sb->s_type->i_mutex_key#4 --> &mm->mmap_sem
> 
> [  459.732165]  Possible unsafe locking scenario:
> 
> [  459.732172]        CPU0                    CPU1
> [  459.732178]        ----                    ----
> [  459.732184]   lock(&mm->mmap_sem);
> [  459.732193]                                lock(&sb->s_type->i_mutex_key#4);
> [  459.732204]                                lock(&mm->mmap_sem);
> [  459.732212]   lock(&queue->mutex);
> [  459.732221]
>                  *** DEADLOCK ***
> 
>>
>> I'm not sure how calling xhci_debugfs_create_endpoint() from
>> xhci_add_endpoint() instead of xhci_check_bandwidth() helps.
>>
>> Both are called with hcd->bandwidth_mutex held:
>>
>> usb_set_interface()
>>          mutex_lock(hcd->bandwidth_mutex);
>>          usb_hcd_alloc_bandwidth()
>>                  hcd->driver->add_endpoint()    -> xhci_add_endpoint()
>>                  hcd->driver->check_bandwidth() -> xhci_check_bandwidth()
>>          mutex_unlock(hcd->bandwidth_mutex);
> 
> Yep, I guess I was lucky not to be able to repro again :)
> 
> The locks involved are:
> 
> hcd->bandwidth_mutex
> mm->mmap_sem
> [uvc] queue->mutex
> 

Ok, took a look at this.
I don't think the bandwidth mutex matters that much.

To my understanding this is caused by the following lock chains:

ucv_queue_mmap()
   mmap_sem --> queue->mutex

uvc_ioctl_streamon() calling usb_set_interface() calling debugfs_create_dir()
   queue->mutex --> i_mutex_key

Some debugfs error case:
   i_mutex_key --> mmap_sem

So we could end up with this deadlock:
CPU0		CPU1		CPU2
mmap_sem	queue->mutex	i_mutex_key
  
waiting for	waiting for	waiting for
queue->mutex	i_mutex_key	mmap_sem	

I have no idea if this can be triggered in real life.

Looks like that requires a some specific debugfs error
to trigger at the same time we are creating a debugfs directory

Thanks
Mathias