linux-kernel - 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 22 Dec 2021 18:32:35 +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: Questions about the patch 054aa8d439b9 ("fget: check that the fd
 still exists after getting a ref to it")

> 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!

-- 
With Best Regards,
Baokun Li
.