Received-SPF: pass (google.com: domain of linux-kernel+bounces-141558-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240411001012.12513-1-torvalds@linux-foundation.org>
 <CAHk-=wiaoij30cnx=jfvg=Br3YTxhQjp4VWRc6=xYE2=+EVRPg@mail.gmail.com>
 <20240411-alben-kocht-219170e9dc99@brauner> <CAHk-=wjrPDx=f5OSnQVbbJ4id6SZk-exB1VW9Uz3R7rKFvTQeQ@mail.gmail.com>
 <CABe3_aGbsPHY9Z5B9WyVWakeWFtief4DpBrDxUiD00qk1irMrg@mail.gmail.com>
 <CABe3_aGGf7kb97gE4FdGmT79Kh5OhbB_2Hqt898WZ+4XGg6j4Q@mail.gmail.com>
 <CABe3_aE_quPE0zKe-p11DF1rBx-+ecJKORY=96WyJ_b+dbxL9A@mail.gmail.com> <CAHk-=wjuzUTH0ZiPe0dAZ4rcVeNoJxhK8Hh_WRBY-ZqM-pGBqg@mail.gmail.com>
In-Reply-To: <CAHk-=wjuzUTH0ZiPe0dAZ4rcVeNoJxhK8Hh_WRBY-ZqM-pGBqg@mail.gmail.com>
From: Charles Mirabile <cmirabil@redhat.com>
Date: Thu, 11 Apr 2024 16:08:20 -0400
Message-ID: <CABe3_aEccnYHm6_pvXKNYkWQ98N9q4JWXTbftgwOMMo+FrmA0Q@mail.gmail.com>
Subject: Re: [PATCH] vfs: relax linkat() AT_EMPTY_PATH - aka flink() - requirements
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Christian Brauner <brauner@kernel.org>, Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>, 
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Andrew Lutomirski <luto@kernel.org>, Peter Anvin <hpa@zytor.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 11, 2024 at 2:14=E2=80=AFPM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 11 Apr 2024 at 10:35, Charles Mirabile <cmirabil@redhat.com> wrot=
e:
> >
> > And a slightly dubious addition to bypass these checks for tmpfiles
> > across the board.
>
> Does this make sense?
>
> I 100% agree that one of the primary reasons why people want flink()
> is that "open tmpfile, finalize contents and permissions, then link
> the final result into the filesystem".
>
> But I would expect that the "same credentials as open" check is the
> one that really matters.

Certainly. I think that in almost all cases, the pattern of preparing
a file and then using linkat to give it its final name will occur
without changing creds and as such your patch will fix it. When
combined with making the CAP_DAC_READ_SEARCH override aware of
namespaces, I think that covers almost all of the remaining edges
cases (i.e. if the difference in creds was actually you becoming more
privileged and not less, then sure  you can do it).

The only possibility that remains is a difference in creds where the
new creds are not privileged. This is the maybe scary situation that
has blocked flink in the past. All I am suggesting is that you can
decompose this niche situation still further into the case where the
file was opened "ordinarily" in some sense which is the case that is
really concerning (oops, when I forked and dropped privileges before
exec, I forgot to set cloexec on one of my fds that points to
something important, and even though I opened it with O_RDONLY, the
unprivileged code is able to make a new link to it in a directory
they control and re-open with O_RDWR because the mode bits allow say
o+w (extremely bizarre and honestly hypothetical in my opinion, but
ok)) and other special situations.

Those situations namely being O_PATH and O_TMPFILE where by
specifying these special flags during open you are indicating that
you intend to do something special with the file descriptor. I think
if either of these flags are present in the file flags, then we are
not in the concerning case, and I think it could be appropriate to
bypass the matching creds check.

>
> And __O_TMPFILE is just a special case that might not even be used -
> it's entirely possible to just do the same with a real file (ie
> non-O_TMPFILE) and link it in place and remove the original.
>
> Not to mention that ->tmpfile() isn't necessarily even available, so
> the whole concept of "use O_TMPFILE and then linkat" is actually
> broken. It *has* to be able to fall back to a regular file to work at
> all on NFS.
>
> So while I understand your motivation, I actually think it's actively
> wrong to special-case __O_TMPFILE, because it encourages a pattern
> that is bad.

The problem with this is that another process might be able to access
the file during via that name during the brief period before it is
unlinked. If I am not using NFS, I am always going to prefer using
O_TMPFILE. I would rather be able to do that without restriction even
if it isn't the most robust solution by your definition.

In my opinion I think it is more robust in the sense that it is truly
atomic and making a named file is the kludge that you have to do to
work around NFS limitations, but I agree that this is a tiny detail
that I certainly do not want to block this patch over because it
already solves the problem I was actually dealing with. Whether or
not it solves this hypothetical problem is less important.

>
>                     Linus
>