Message-ID: <41f2809e-459c-a179-b5b1-62d5a0821574@redhat.com>
Date:   Sun, 2 Jan 2022 21:35:22 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [lockdep] UAF read in print_name().
Content-Language: en-US
From:   Waiman Long <longman@redhat.com>
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, Christoph Hellwig <hch@lst.de>
References: <77f05c15-81b6-bddd-9650-80d5f23fe330@i-love.sakura.ne.jp>
 <015af849-3571-e9ac-692f-d803aa19f566@redhat.com>
 <f80074eb-58bc-7db7-d945-ef18f7617c4e@i-love.sakura.ne.jp>
 <843bffdd-6c5b-2869-e089-01d180f36a76@redhat.com>
In-Reply-To: <843bffdd-6c5b-2869-e089-01d180f36a76@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 1/1/22 13:02, Waiman Long wrote:
> On 12/30/21 10:09, Tetsuo Handa wrote:
>> On 2021/12/29 12:25, Waiman Long wrote:
>>> On 12/28/21 05:49, Tetsuo Handa wrote:
>>>> Hello.
>>>>
>>>> I found using linux-next-20211210 that reading /proc/lockdep after 
>>>> lockdep splat
>>>> triggers UAF read access. I think this is a side effect of zapping 
>>>> dependency
>>>> information when loop driver's WQ is destroyed. You might want to 
>>>> xchg() the pointer
>>>> with a dummy struct containing a static string.
>>>>
>>>> difference before lockdep splat and after lockdep splat
>>>> ----------------------------------------
>>>> 8635c8636
>>>> < ffff88811561cd28 OPS:      26 FD:  122 BD:    1 +.+.: 
>>>> (wq_completion)loop0
>>>> ---
>>>>> ffff88811561cd28 OPS:      31 FD: 439 BD:    1 +.+.:  
>>>>> M>^MM-^AM-^HM-^?M-^?
>>> Thanks for reporting.
>>>
>>> Yes, listing locking classes by /proc/lockdep is racy as 
>>> all_lock_classes is accessed
>>> without lock protection. OTOH, we probably can't fix this race as 
>>> lock hold time will be
>>> too long for this case. Atomically xchg the class name is a possible 
>>> workaround, but we
>>> also need to add additional checks as the iteration may also be 
>>> redirected to
>>> free_lock_classes leading to an endless iteration loop.
>> Thanks for responding. But is this bug really unfixable?
> I am not saying that it is unfixable. I am just saying that we cannot 
> guarantee a consistent output of /proc/lockdep as internal data may 
> change in the middle of dumping the output.
>>
>> Please see the following result.
>>
>> ----------------------------------------
>> [root@localhost ~]# uname -r
>> 5.16.0-rc4-next-20211210
>> [root@localhost ~]# grep loop /proc/lockdep
>> [root@localhost ~]# truncate -s 100m testfile
>> [root@localhost ~]# losetup -f testfile
>> [root@localhost ~]# grep loop /proc/lockdep
>> ffffffffa02b73c8 OPS:      17 FD:   34 BD:    1 +.+.: loop_ctl_mutex
>> ffff888106fb0528 OPS:     114 FD:  183 BD:    1 +.+.: 
>> (wq_completion)loop0
>> [root@localhost ~]# losetup -D
>> [root@localhost ~]# grep loop /proc/lockdep
>> ffffffffa02b73c8 OPS:      17 FD:   34 BD:    1 +.+.: loop_ctl_mutex
>> ffffffffa02b7328 OPS:       1 FD:    1 BD:    1 +.+.: 
>> loop_validate_mutex
>> [root@localhost ~]# losetup -f testfile
>> [root@localhost ~]# grep loop /proc/lockdep
>> ffffffffa02b73c8 OPS:      18 FD:   34 BD:    1 +.+.: loop_ctl_mutex
>> ffffffffa02b7328 OPS:       1 FD:    1 BD:    1 +.+.: 
>> loop_validate_mutex
>> ffff888106fb1128 OPS:     118 FD:  183 BD:    1 +.+.: 
>> (wq_completion)loop0
>> [root@localhost ~]# losetup -D
>> [root@localhost ~]# grep loop /proc/lockdep
>> ffffffffa02b73c8 OPS:      18 FD:   34 BD:    1 +.+.: loop_ctl_mutex
>> ffffffffa02b7328 OPS:       2 FD:    1 BD:    1 +.+.: 
>> loop_validate_mutex
>> [root@localhost ~]# grep debug_locks /proc/lockdep_stats
>>   debug_locks:                             1
>> [root@localhost ~]#
>> ----------------------------------------
>>
>> We can confirm that the "(wq_completion)loop0" entry disappears when 
>> WQ for /dev/loop0 is destroyed.
>>
>> Then, please see the following reproducer for this lockdep problem. 
>> As you can see, there is 10
>> seconds between lockdep complained and /proc/lockdep is read. 10 
>> seconds should be enough time
>> for erasing the "(wq_completion)loop0" entry.
>
> Thanks for the reproducer.

Your reproducer can always reproduce the problem. It turns out that it 
is not really a race condition. The UAF problem is caused by the failure 
of lockdep to properly zap the "(wq_completion)loop0" lock class. I am 
going to send out a patch to address this bug.

Cheers,
Longman