linux-kernel - Re: Questions about the patch 054aa8d439b9 ("fget: check that the fd still exists after getting a ref to it")

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 10 Jan 2022 09:25:54 +0800
From:   "libaokun (A)" <libaokun1@...wei.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        <mszeredi@...hat.com>, <jannh@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
CC:     "zhangyi (F)" <yi.zhang@...wei.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Baokun Li <libaokun1@...wei.com>
Subject: Re: Questions about the patch 054aa8d439b9 ("fget: check that the fd
 still exists after getting a ref to it")

Happy New Year!

ping

在 2021/12/22 18:32, libaokun (A) 写道:
>> From: Linus Torvalds <torvalds@...ux-foundation.org>
>> Date: Wed, 1 Dec 2021 10:06:14 -0800
>> Subject: fget: check that the fd still exists after getting a ref to it
>>
>> Jann Horn points out that there is another possible race wrt Unix domain
>> socket garbage collection, somewhat reminiscent of the one fixed in
>> commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK").
>>
>> See the extended comment about the garbage collection requirements added
>> to unix_peek_fds() by that commit for details.
>>
>> The race comes from how we can locklessly look up a file descriptor just
>> as it is in the process of being closed, and with the right artificial
>> timing (Jann added a few strategic 'mdelay(500)' calls to do that), the
>> Unix domain socket garbage collector could see the reference count
>> decrement of the close() happen before fget() took its reference to the
>> file and the file was attached onto a new file descriptor.
>
> I analyzed this CVE and tried to reproduce it.
>
> I guess he triggered it like the stack below.
>
>
> close_fd                               |
>  pick_file                             |
>                                        | __fget_files
> file = files_lookup_fd_rcu(files, fd); |
>                                        | 
> rcu_assign_pointer(fdt->fd[fd], NULL);
>  filp_close                            |
>   fput                                 |
>                                        | get_file_rcu_many // ned ref>=1
>    fput_many(file, 1);                 |
>     file_free(file);                   |
>                                        |  return file
>                                        |  // read-after-free
>
>
>
> If you want to successfully execute the get_file_rcu_many function,
>
> the reference counting of the file is greater than or equal to 1 and
>
> is greater than or equal to 2 after the execution.
>
> However, close releases only one reference count and does not release 
> the file,
>
> so read-after-free does not occur. So how is the race triggered here?
>
> The question has been pondered for a long time without any results.
>
> Could I get more details (e.g. reproduction methods or stacks) from you ?
>
> I would appreciate it if you could help me.
>
>
>> This is all (intentionally) correct on the 'struct file *' side, with
>> RCU lookups and lockless reference counting very much part of the
>> design. Getting that reference count out of order isn't a problem per
>> se.
>>
>> But the garbage collector can get confused by seeing this situation of
>> having seen a file not having any remaining external references and then
>> seeing it being attached to an fd.
>>
>> In commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK") the
>> fix was to serialize the file descriptor install with the garbage
>> collector by taking and releasing the unix_gc_lock.
>>
>> That's not really an option here, but since this all happens when we are
>> in the process of looking up a file descriptor, we can instead simply
>> just re-check that the file hasn't been closed in the meantime, and just
>> re-do the lookup if we raced with a concurrent close() of the same file
>> descriptor.
>>
>> Reported-and-tested-by: Jann Horn <jannh@...gle.com>
>> Acked-by: Miklos Szeredi <mszeredi@...hat.com>
>> Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
>> ---
>> fs/file.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/file.c b/fs/file.c
>> index 8627dacfc4246..ad4a8bf3cf109 100644
>> --- a/fs/file.c
>> +++ b/fs/file.c
>> @@ -858,6 +858,10 @@ loop:
>> file = NULL;
>> else if (!get_file_rcu_many(file, refs))
>> goto loop;
>> + else if (files_lookup_fd_raw(files, fd) != file) {
>> + fput_many(file, refs);
>> + goto loop;
>> + }
>> }
>> rcu_read_unlock();
>> -- cgit 1.2.3-1.el7
>
> Looking forward to hearing from you.
>
> Thank you!
>