linux-kernel - Re: race between vfs_rename and do

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: <CAJfpegsQO-35p6uoG2ZfuCOLPFwnkbTcLc3K8r+HiS2un9au_w@mail.gmail.com>
Date:   Wed, 16 Feb 2022 14:37:44 +0100
From:   Miklos Szeredi <miklos@...redi.hu>
To:     Xavier Roche <xavier.roche@...olia.com>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Matthew Wilcox <willy@...radead.org>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
Subject: Re: race between vfs_rename and do_linkat (mv and link)

On Wed, 16 Feb 2022 at 14:18, Xavier Roche <xavier.roche@...olia.com> wrote:
>
> On Wed, Feb 16, 2022 at 11:28:18AM +0100, Miklos Szeredi wrote:
> > Something like this:
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 3f1829b3ab5b..dd6908cee49d 100644
>
> Tested-by: Xavier Roche <xavier.roche@...olia.com>
>
> I confirm this completely fixes at least the specific race. Tested on a
> unpatched and then patched 5.16.5, with the trivial bash test, and then
> with a C++ torture test.

Thanks for testing.

One issue with the patch is nesting of lock_rename() calls in stacked
fs (rwsem is not allowed to recurse even for read locks).

So the lock needs to be per-sb, but then do_linkat() becomes more
complex due to not being able to use the filename_create() helper.
But it's still much simpler than the special lookup loop described by
Al.

Thanks,
Miklos
>
> Before:
> -------
>
> $ time ./linkbug
> Failed after 4 with No such file or directory
>
> real    0m0,004s
> user    0m0,000s
> sys     0m0,004s
>
> After:
> ------
>
> (no error after ten minutes of running the program)
>
> Torture test program:
> ---------------------
>
> /* Linux rename vs. linkat race condition.
>  * Rationale: both (1) moving a file to a target and (2) linking the target to a file in parallel leads to a race
>  * on Linux kernel.
>  * Sample file courtesy of Xavier Grand at Algolia
>  * g++ -pthread linkbug.c -o linkbug
>  */
>
> #include <thread>
> #include <unistd.h>
> #include <assert.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <iostream>
> #include <string.h>
>
> static const char* producedDir = "/tmp";
> static const char* producedFile = "/tmp/file.txt";
> static const char* producedTmpFile = "/tmp/file.txt.tmp";
> static const char* producedThreadDir = "/tmp/tmp";
> static const char* producedThreadFile = "/tmp/file.txt.tmp.2";
>
> bool createFile(const char* path)
> {
>     const int fdOut = open(path,
>                            O_WRONLY | O_CREAT | O_TRUNC | O_EXCL | O_CLOEXEC,
>                            S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
>     assert(fdOut != -1);
>     assert(write(fdOut, "Foo", 4) == 4);
>     assert(close(fdOut) == 0);
>     return true;
> }
>
> void func()
> {
>     int nbSuccess = 0;
>     // Loop producedThread a hardlink of the file
>     while (true) {
>         if (link(producedFile, producedThreadFile) != 0) {
>             std::cout << "Failed after " << nbSuccess << " with " << strerror(errno) << std::endl;
>             exit(EXIT_FAILURE);
>         } else {
>             nbSuccess++;
>         }
>         assert(unlink(producedThreadFile) == 0);
>     }
> }
>
> int main()
> {
>     // Setup env
>     unlink(producedTmpFile);
>     unlink(producedFile);
>     unlink(producedThreadFile);
>     createFile(producedFile);
>     mkdir(producedThreadDir, 0777);
>
>     // Async thread doing a hardlink and moving it
>     std::thread t(func);
>     // Loop creating a .tmp and moving it
>     while (true) {
>         assert(createFile(producedTmpFile));
>         assert(rename(producedTmpFile, producedFile) == 0);
>     }
>     return 0;
> }