"The definition of insanity is doing the same thing over and over
again and expecting different results”
We've tried to do this before, most recently with commit bb2314b47996
("fs: Allow unprivileged linkat(..., AT_EMPTY_PATH) aka flink") about a
decade ago.
But the effort goes back even further than that, eg this thread back
from 1998 that is so old that we don't even have it archived in lore:
https://lkml.org/lkml/1998/3/10/108
which also points out some of the reasons why it's dangerous.
Or, how about then in 2003:
https://lkml.org/lkml/2003/4/6/112
where we went through some of the same arguments, just wirh different
people involved.
In particular, having access to a file descriptor does not necessarily
mean that you have access to the path that was used for lookup, and
there may be very good reasons why you absolutely must not have access
to a path to said file.
For example, if we were passed a file descriptor from the outside into
some limited environment (think chroot, but also user namespaces etc) a
'flink()' system call could now make that file visible inside a context
where it's not supposed to be visible.
In the process the user may also be able to re-open it with permissions
that the original file descriptor did not have (eg a read-only file
descriptor may be associated with an underlying file that is writable).
Another variation on this is if somebody else (typically root) opens a
file in a directory that is not accessible to others, and passes the
file descriptor on as a read-only file. Again, the access to the file
descriptor does not imply that you should have access to a path to the
file in the filesystem.
So while we have tried this several times in the past, it never works.
The last time we did this, that commit bb2314b47996 quickly got reverted
again in commit f0cc6ffb8ce8 (Revert "fs: Allow unprivileged linkat(...,
AT_EMPTY_PATH) aka flink"), with a note saying "We may re-do this once
the whole discussion about the interface is done".
Well, the discussion is long done, and didn't come to any resolution.
There's no question that 'flink()' would be a useful operation, but it's
a dangerous one.
However, it does turn out that since 2008 (commit d76b0d9b2d87: "CRED:
Use creds in file structs") we have had a fairly straightforward way to
check whether the file descriptor was opened by the same credentials as
the credentials of the flink().
That allows the most common patterns that people want to use, which tend
to be to either open the source carefully (ie using the openat2()
RESOLVE_xyz flags, and/or checking ownership with fstat() before
linking), or to use O_TMPFILE and fill in the file contents before it's
exposed to the world with linkat().
But it also means that if the file descriptor was opened by somebody
else, or we've gone through a credentials change since, the operation no
longer works (unless we have CAP_DAC_READ_SEARCH capabilities, as
before).
Note that the credential equality check is done by using pointer
equality, which means that it's not enough that you have effectively the
same user - they have to be literally identical, since our credentials
are using copy-on-write semantics.
So you can't change your credentials to something else and try to change
it back to the same ones between the open() and the linkat(). This is
not meant to be some kind of generic permission check, this is literally
meant as a "the open and link calls are 'atomic' wrt user credentials"
check.
It also means that you can't just move things between namespaces,
because the credentials aren't just a list of uid's and gid's: they
includes the pointer to the user_ns that the capabilities are relative
to.
So let's try this one more time and see if maybe this approach ends up
being workable after all.
Cc: Andrew Lutomirski <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Peter Anvin <[email protected]>
Cc: Jan Kara <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
---
fs/namei.c | 17 ++++++++++++-----
include/linux/namei.h | 1 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index c5b2a25be7d0..3c684014eb40 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2422,6 +2422,14 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
if (!f.file)
return ERR_PTR(-EBADF);
+ if (flags & LOOKUP_DFD_MATCH_CREDS) {
+ if (f.file->f_cred != current_cred() &&
+ !capable(CAP_DAC_READ_SEARCH)) {
+ fdput(f);
+ return ERR_PTR(-ENOENT);
+ }
+ }
+
dentry = f.file->f_path.dentry;
if (*s && unlikely(!d_can_lookup(dentry))) {
@@ -4641,14 +4649,13 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
goto out_putnames;
}
/*
- * To use null names we require CAP_DAC_READ_SEARCH
+ * To use null names we require CAP_DAC_READ_SEARCH or
+ * that the open-time creds of the dfd matches current.
* This ensures that not everyone will be able to create
* handlink using the passed filedescriptor.
*/
- if (flags & AT_EMPTY_PATH && !capable(CAP_DAC_READ_SEARCH)) {
- error = -ENOENT;
- goto out_putnames;
- }
+ if (flags & AT_EMPTY_PATH)
+ how |= LOOKUP_DFD_MATCH_CREDS;
if (flags & AT_SYMLINK_FOLLOW)
how |= LOOKUP_FOLLOW;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 74e0cc14ebf8..678ffe4acf99 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -44,6 +44,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
#define LOOKUP_BENEATH 0x080000 /* No escaping from starting point. */
#define LOOKUP_IN_ROOT 0x100000 /* Treat dirfd as fs root. */
#define LOOKUP_CACHED 0x200000 /* Only do cached lookup */
+#define LOOKUP_DFD_MATCH_CREDS 0x400000 /* Require that dfd creds match current */
/* LOOKUP_* flags which do scope-related checks based on the dirfd. */
#define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT)
--
2.44.0.330.g4d18c88175
On Wed, 10 Apr 2024 at 17:10, Linus Torvalds
<[email protected]> wrote:
>
> + if (flags & LOOKUP_DFD_MATCH_CREDS) {
> + if (f.file->f_cred != current_cred() &&
> + !capable(CAP_DAC_READ_SEARCH)) {
> + fdput(f);
> + return ERR_PTR(-ENOENT);
> + }
> + }
Side note: I suspect that this could possibly be relaxed further, by
making the rule be that if something has been explicitly opened to be
used as a path (ie O_PATH was used at open time), we can link to it
even across different credentials.
IOW, the above could perhaps even be
+ if (flags & LOOKUP_DFD_MATCH_CREDS) {
+ if (!(f.file->f_mode & FMODE_PATH) &&
+ f.file->f_cred != current_cred() &&
+ !capable(CAP_DAC_READ_SEARCH)) {
+ fdput(f);
+ return ERR_PTR(-ENOENT);
+ }
+ }
which would _allow_ people to pass in paths as file descriptors if
they actually wanted to.
After all, the only thing you can do with an O_PATH file descriptor is
to use it as a path - there would be no other reason to use O_PATH in
the first place. So if you now pass it to somebody else, clearly you
are intentionally trying to make it available *as* a path.
So you could imagine doing something like this:
// Open path as root
int fd = open('filename", O_PATH);
// drop privileges
// setresuid(..) or chmod() or enter new namespace or whatever
linkat(fd, "", AT_FDCWD, "newname", AT_EMPTY_PATH);
and it would open the path with one set of privileges, but then
intentionally go into a more restricted mode and create a link to the
source within that restricted environment.
Sensible? Who knows. I'm just throwing this out as another "this may
be the solution to our historical flink() issues".
Linus
On Wed, Apr 10, 2024 at 07:39:49PM -0700, Linus Torvalds wrote:
> On Wed, 10 Apr 2024 at 17:10, Linus Torvalds
> <[email protected]> wrote:
> >
> > + if (flags & LOOKUP_DFD_MATCH_CREDS) {
> > + if (f.file->f_cred != current_cred() &&
> > + !capable(CAP_DAC_READ_SEARCH)) {
> > + fdput(f);
> > + return ERR_PTR(-ENOENT);
> > + }
> > + }
>
> Side note: I suspect that this could possibly be relaxed further, by
> making the rule be that if something has been explicitly opened to be
> used as a path (ie O_PATH was used at open time), we can link to it
> even across different credentials.
I had a similar discussion a while back someone requested that we relax
permissions so linkat can be used in containers. And I drafted the
following patch back then:
https://lore.kernel.org/all/20231113-undenkbar-gediegen-efde5f1c34bc@brauner
IOW, I had intended to make this work with containers so that we check
CAP_DAC_READ_SEARCH in the namespace of the opener of the file. My
thinking had been that this can serve as a way to say "Hey, I could've
opened this file in the openers namespace therefore let me make a path
to it.". I didn't actually send it because I thought the original author
would but imho, that would be a worthwhile addition to your patch if
this makes sense...
>
> IOW, the above could perhaps even be
>
> + if (flags & LOOKUP_DFD_MATCH_CREDS) {
> + if (!(f.file->f_mode & FMODE_PATH) &&
> + f.file->f_cred != current_cred() &&
> + !capable(CAP_DAC_READ_SEARCH)) {
> + fdput(f);
> + return ERR_PTR(-ENOENT);
> + }
> + }
>
> which would _allow_ people to pass in paths as file descriptors if
> they actually wanted to.
>
> After all, the only thing you can do with an O_PATH file descriptor is
> to use it as a path - there would be no other reason to use O_PATH in
> the first place. So if you now pass it to somebody else, clearly you
> are intentionally trying to make it available *as* a path.
>
> So you could imagine doing something like this:
>
> // Open path as root
> int fd = open('filename", O_PATH);
>
> // drop privileges
> // setresuid(..) or chmod() or enter new namespace or whatever
>
> linkat(fd, "", AT_FDCWD, "newname", AT_EMPTY_PATH);
>
> and it would open the path with one set of privileges, but then
> intentionally go into a more restricted mode and create a link to the
> source within that restricted environment.
>
> Sensible? Who knows. I'm just throwing this out as another "this may
> be the solution to our historical flink() issues".
>
> Linus
On Thu, Apr 11, 2024 at 11:04:59AM +0200, Christian Brauner wrote:
> On Wed, Apr 10, 2024 at 07:39:49PM -0700, Linus Torvalds wrote:
> > On Wed, 10 Apr 2024 at 17:10, Linus Torvalds
> > <[email protected]> wrote:
> > >
> > > + if (flags & LOOKUP_DFD_MATCH_CREDS) {
> > > + if (f.file->f_cred != current_cred() &&
> > > + !capable(CAP_DAC_READ_SEARCH)) {
> > > + fdput(f);
> > > + return ERR_PTR(-ENOENT);
> > > + }
> > > + }
> >
> > Side note: I suspect that this could possibly be relaxed further, by
> > making the rule be that if something has been explicitly opened to be
> > used as a path (ie O_PATH was used at open time), we can link to it
> > even across different credentials.
>
> I had a similar discussion a while back someone requested that we relax
> permissions so linkat can be used in containers. And I drafted the
> following patch back then:
>
> https://lore.kernel.org/all/20231113-undenkbar-gediegen-efde5f1c34bc@brauner
>
> IOW, I had intended to make this work with containers so that we check
> CAP_DAC_READ_SEARCH in the namespace of the opener of the file. My
> thinking had been that this can serve as a way to say "Hey, I could've
> opened this file in the openers namespace therefore let me make a path
> to it.". I didn't actually send it because I thought the original author
> would but imho, that would be a worthwhile addition to your patch if
> this makes sense...
For example, say someone opened an O_PATH fd in the initial user ns and
then send that file over an AF_UNIX socket to some other container the
ns_capable(f_cred->user_ns, CAP_DAC_READ_SEARCH) would always be false.
The other way around though would work. Which imho is exactly what we
want to make such cross-container interactions with linkat() safe.
And this didn't aim to solve the problem of allowing unprivileged users
in the initial namespace to do linkat(), of course which yours does.
Btw, I think we should try to avoid putting this into path_init() and
confine this to linkat() itself imho. The way I tried to do it was by
presetting a root for filename_lookup(); means we also don't need a
LOOKUP_* flag for this as this is mostly a linkat thing.
So maybe your suggestion combined with my own attempt would make this
work for unprivileged users and containers?
if (f.file->f_cred != current_cred() &&
!ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH))
Worst case we get a repeat of the revert and get to make this a 10 year
anniversary patch attempt?
On Thu, 11 Apr 2024 at 02:05, Christian Brauner <[email protected]> wrote:
>
> I had a similar discussion a while back someone requested that we relax
> permissions so linkat can be used in containers.
Hmm.
Ok, that's different - it just wants root to be able to do it, but
"root" being just in the container itself.
I don't think that's all that useful - I think one of the issues with
linkat(AT_EMPTY_PATH) is exactly that "it's only useful for root",
which means that it's effectively useless. Inside a container or out.
Because very few loads run as root-only (and fewer still run with any
capability bits that aren't just "root or nothing").
Before I did all this, I did a Debian code search for linkat with
AT_EMPTY_PATH, and it's almost non-existent. And I think it's exactly
because of this "when it's only useful for root, it's hardly useful at
all" issue.
(Of course, my Debian code search may have been broken).
So I suspect your special case is actually largely useless, and what
the container user actually wanted was what my patch does, but they
didn't think that was possible, so they asked to just extend the
"root" notion.
I've added Charles to the Cc.
But yes, with my patch, it would now be trivial to make that
capable(CAP_DAC_READ_SEARCH)
test also be
ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH)
instead. I suspect not very many would care any more, but it does seem
conceptually sensible.
As to your patch - I don't like your nd->root games in that patch at
all. That looks odd.
Yes, it makes lookup ignore the dfd (so you avoid the TOCTOU issue),
but it also makes lookup ignore "/". Which happens to be ok with an
empty path, but still...
So it feels to me like that patch of yours mis-uses something that is
just meant for vfs_path_lookup().
It may happen to work, but it smells really odd to me.
Linus
On Thu, Apr 11, 2024 at 12:15 PM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, 11 Apr 2024 at 02:05, Christian Brauner <[email protected]> wrote:
> >
> > I had a similar discussion a while back someone requested that we relax
> > permissions so linkat can be used in containers.
>
> Hmm.
>
> Ok, that's different - it just wants root to be able to do it, but
> "root" being just in the container itself.
>
> I don't think that's all that useful - I think one of the issues with
> linkat(AT_EMPTY_PATH) is exactly that "it's only useful for root",
> which means that it's effectively useless. Inside a container or out.
>
> Because very few loads run as root-only (and fewer still run with any
> capability bits that aren't just "root or nothing").
>
> Before I did all this, I did a Debian code search for linkat with
> AT_EMPTY_PATH, and it's almost non-existent. And I think it's exactly
> because of this "when it's only useful for root, it's hardly useful at
> all" issue.
>
> (Of course, my Debian code search may have been broken).
>
> So I suspect your special case is actually largely useless, and what
> the container user actually wanted was what my patch does, but they
> didn't think that was possible, so they asked to just extend the
> "root" notion.
>
Yes, that is absolutely the case. When Christian poked holes in my
initial submission, I started working on a v2 but haven't had a chance
to send it because I wanted to make sure my arguments etc were
airtight because I am well aware of just how long and storied the
history of flink is. In the v2 that I didn't post yet, I extended the
ability to link *any* file from only true root to also "root" within a
container following the potentially suspect approach that christian
suggested (I see where you are coming from with the unobvious approach
to avoiding toctou though I obviously support this patch being
merged), but I added a followup patch that checks for whether the file
in question is an `__O_TMPFILE` file which is trivial once we are
jumping through the hoops of looking up the struct file. If it is a
tmpfile (i.e. linkcount = 0) I think that all the concerns raised
about processes that inherit a fd being able to create links to the
file inappropriately are moot. Here is an excerpt from the cover
letter I was planning to send that explains my reasoning.
- the ability to create an unnamed file, adjust its contents
and attributes (i.e. uid, gid, mode, xattrs, etc) and then only give it a name
once they are ready is exactly the ideal use-case for a hypothetical `flink`
system call. The ability to use `AT_EMPTY_PATH` with `linkat` essentially turns
it into `flink`, but these restrictions hamper it from actually being used, as
most code is not privileged. By checking whether the file to be linked is a
temporary (i.e. unnamed) file we can allow this useful case without allowing
the more risky cases that could have security implications.
- Although it might appear that the security posture is affected, it is not.
Callers who open an extant file on disk in read only mode for sharing with
potentially untrustworthy code can trust that this change will not affect them
because it only applies to temporary files. Callers who open a temporary file
need to do so with write access if they want to actually put any data in it,
so they will already have to reopen the file (e.g. by linking it to a path
and opening that path, or opening the magic symlink in proc) if they want to
share it in read-only mode with untrustworthy code. As such, that new file
description will no longer be marked as a temporary file and thus these changes
do not apply. The final case I can think of is the unlikely possibility of
intentionally sharing read write access to data stored in a temporary file with
untrustworthy code, but where that code being able to make a link would still
be deleterious. In such a bizarre case, the `O_EXCL` can and should be used to
fully prevent the temporary file from ever having a name, and this change does
not alter its efficacy.
> I've added Charles to the Cc.
>
> But yes, with my patch, it would now be trivial to make that
>
> capable(CAP_DAC_READ_SEARCH)
>
> test also be
>
> ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH)
>
> instead. I suspect not very many would care any more, but it does seem
> conceptually sensible.
>
> As to your patch - I don't like your nd->root games in that patch at
> all. That looks odd.
>
> Yes, it makes lookup ignore the dfd (so you avoid the TOCTOU issue),
> but it also makes lookup ignore "/". Which happens to be ok with an
> empty path, but still...
>
> So it feels to me like that patch of yours mis-uses something that is
> just meant for vfs_path_lookup().
>
> It may happen to work, but it smells really odd to me.
>
> Linus
>
Here is an updated version of linus's patch that makes the check
namespace relative
---
fs/namei.c | 17 ++++++++++++-----
include/linux/namei.h | 1 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 4e0de939fea1..2bcc10976ba7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2419,6 +2419,14 @@ static const char *path_init(struct nameidata
*nd, unsigned flags)
if (!f.file)
return ERR_PTR(-EBADF);
+ if (flags & LOOKUP_DFD_MATCH_CREDS) {
+ if (f.file->f_cred != current_cred() &&
+ !ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH)) {
+ fdput(f);
+ return ERR_PTR(-ENOENT);
+ }
+ }
+
dentry = f.file->f_path.dentry;
if (*s && unlikely(!d_can_lookup(dentry))) {
@@ -4640,14 +4648,13 @@ int do_linkat(int olddfd, struct filename
*old, int newdfd,
goto out_putnames;
}
/*
- * To use null names we require CAP_DAC_READ_SEARCH
+ * To use null names we require CAP_DAC_READ_SEARCH or
+ * that the open-time creds of the dfd matches current.
* This ensures that not everyone will be able to create
* handlink using the passed filedescriptor.
*/
- if (flags & AT_EMPTY_PATH && !capable(CAP_DAC_READ_SEARCH)) {
- error = -ENOENT;
- goto out_putnames;
- }
+ if (flags & AT_EMPTY_PATH)
+ how |= LOOKUP_DFD_MATCH_CREDS;
if (flags & AT_SYMLINK_FOLLOW)
how |= LOOKUP_FOLLOW;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 74e0cc14ebf8..678ffe4acf99 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -44,6 +44,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
#define LOOKUP_BENEATH 0x080000 /* No escaping from starting point. */
#define LOOKUP_IN_ROOT 0x100000 /* Treat dirfd as fs root. */
#define LOOKUP_CACHED 0x200000 /* Only do cached lookup */
+#define LOOKUP_DFD_MATCH_CREDS 0x400000 /* Require that dfd creds
match current */
/* LOOKUP_* flags which do scope-related checks based on the dirfd. */
#define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT)
--
2.44.0
On Thu, Apr 11, 2024 at 12:44 PM Charles Mirabile <[email protected]> wrote:
>
> On Thu, Apr 11, 2024 at 12:15 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Thu, 11 Apr 2024 at 02:05, Christian Brauner <[email protected]> wrote:
> > >
> > > I had a similar discussion a while back someone requested that we relax
> > > permissions so linkat can be used in containers.
> >
> > Hmm.
> >
> > Ok, that's different - it just wants root to be able to do it, but
> > "root" being just in the container itself.
> >
> > I don't think that's all that useful - I think one of the issues with
> > linkat(AT_EMPTY_PATH) is exactly that "it's only useful for root",
> > which means that it's effectively useless. Inside a container or out.
> >
> > Because very few loads run as root-only (and fewer still run with any
> > capability bits that aren't just "root or nothing").
> >
> > Before I did all this, I did a Debian code search for linkat with
> > AT_EMPTY_PATH, and it's almost non-existent. And I think it's exactly
> > because of this "when it's only useful for root, it's hardly useful at
> > all" issue.
> >
> > (Of course, my Debian code search may have been broken).
> >
> > So I suspect your special case is actually largely useless, and what
> > the container user actually wanted was what my patch does, but they
> > didn't think that was possible, so they asked to just extend the
> > "root" notion.
> >
> Yes, that is absolutely the case. When Christian poked holes in my
> initial submission, I started working on a v2 but haven't had a chance
> to send it because I wanted to make sure my arguments etc were
> airtight because I am well aware of just how long and storied the
> history of flink is. In the v2 that I didn't post yet, I extended the
> ability to link *any* file from only true root to also "root" within a
> container following the potentially suspect approach that christian
> suggested (I see where you are coming from with the unobvious approach
> to avoiding toctou though I obviously support this patch being
> merged), but I added a followup patch that checks for whether the file
> in question is an `__O_TMPFILE` file which is trivial once we are
> jumping through the hoops of looking up the struct file. If it is a
> tmpfile (i.e. linkcount = 0) I think that all the concerns raised
> about processes that inherit a fd being able to create links to the
> file inappropriately are moot. Here is an excerpt from the cover
> letter I was planning to send that explains my reasoning.
>
> - the ability to create an unnamed file, adjust its contents
> and attributes (i.e. uid, gid, mode, xattrs, etc) and then only give it a name
> once they are ready is exactly the ideal use-case for a hypothetical `flink`
> system call. The ability to use `AT_EMPTY_PATH` with `linkat` essentially turns
> it into `flink`, but these restrictions hamper it from actually being used, as
> most code is not privileged. By checking whether the file to be linked is a
> temporary (i.e. unnamed) file we can allow this useful case without allowing
> the more risky cases that could have security implications.
>
> - Although it might appear that the security posture is affected, it is not.
> Callers who open an extant file on disk in read only mode for sharing with
> potentially untrustworthy code can trust that this change will not affect them
> because it only applies to temporary files. Callers who open a temporary file
> need to do so with write access if they want to actually put any data in it,
> so they will already have to reopen the file (e.g. by linking it to a path
> and opening that path, or opening the magic symlink in proc) if they want to
> share it in read-only mode with untrustworthy code. As such, that new file
> description will no longer be marked as a temporary file and thus these changes
> do not apply. The final case I can think of is the unlikely possibility of
> intentionally sharing read write access to data stored in a temporary file with
> untrustworthy code, but where that code being able to make a link would still
> be deleterious. In such a bizarre case, the `O_EXCL` can and should be used to
> fully prevent the temporary file from ever having a name, and this change does
> not alter its efficacy.
>
> > I've added Charles to the Cc.
> >
> > But yes, with my patch, it would now be trivial to make that
> >
> > capable(CAP_DAC_READ_SEARCH)
> >
> > test also be
> >
> > ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH)
> >
> > instead. I suspect not very many would care any more, but it does seem
> > conceptually sensible.
> >
> > As to your patch - I don't like your nd->root games in that patch at
> > all. That looks odd.
> >
> > Yes, it makes lookup ignore the dfd (so you avoid the TOCTOU issue),
> > but it also makes lookup ignore "/". Which happens to be ok with an
> > empty path, but still...
> >
> > So it feels to me like that patch of yours mis-uses something that is
> > just meant for vfs_path_lookup().
> >
> > It may happen to work, but it smells really odd to me.
> >
> > Linus
> >
And a slightly dubious addition to bypass these checks for tmpfiles
across the board.
I don't like putting the details of this in the path lookup code, but
it is definitely nicer here than
looking up the fd twice, once here and again in do_linkat
These changes not strictly necessary, because the existing patch
allows unprivileged
tmpfile flink as long as the creds don't change, but I do think that
my reasoning is sound
that linking a tmpfile should always be ok whether or not creds have changed
---
fs/namei.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/namei.c b/fs/namei.c
index 2bcc10976ba7..5c9fabc39481 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2421,6 +2421,7 @@ static const char *path_init(struct nameidata
*nd, unsigned flags)
if (flags & LOOKUP_DFD_MATCH_CREDS) {
if (f.file->f_cred != current_cred() &&
+ !(f.file->f_flags & __O_TMPFILE) &&
!ns_capable(f.file->f_cred->user_ns,
CAP_DAC_READ_SEARCH)) {
fdput(f);
return ERR_PTR(-ENOENT);
On Thu, Apr 11, 2024 at 1:29 PM Charles Mirabile <[email protected]> wrote:
>
> Here is an updated version of linus's patch that makes the check
> namespace relative
> ---
> fs/namei.c | 17 ++++++++++++-----
> include/linux/namei.h | 1 +
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 4e0de939fea1..2bcc10976ba7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2419,6 +2419,14 @@ static const char *path_init(struct nameidata
> *nd, unsigned flags)
> if (!f.file)
> return ERR_PTR(-EBADF);
>
> + if (flags & LOOKUP_DFD_MATCH_CREDS) {
> + if (f.file->f_cred != current_cred() &&
> + !ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH)) {
> + fdput(f);
> + return ERR_PTR(-ENOENT);
> + }
> + }
> +
> dentry = f.file->f_path.dentry;
>
> if (*s && unlikely(!d_can_lookup(dentry))) {
> @@ -4640,14 +4648,13 @@ int do_linkat(int olddfd, struct filename
> *old, int newdfd,
> goto out_putnames;
> }
> /*
> - * To use null names we require CAP_DAC_READ_SEARCH
> + * To use null names we require CAP_DAC_READ_SEARCH or
> + * that the open-time creds of the dfd matches current.
> * This ensures that not everyone will be able to create
> * handlink using the passed filedescriptor.
> */
> - if (flags & AT_EMPTY_PATH && !capable(CAP_DAC_READ_SEARCH)) {
> - error = -ENOENT;
> - goto out_putnames;
> - }
> + if (flags & AT_EMPTY_PATH)
> + how |= LOOKUP_DFD_MATCH_CREDS;
>
> if (flags & AT_SYMLINK_FOLLOW)
> how |= LOOKUP_FOLLOW;
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 74e0cc14ebf8..678ffe4acf99 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -44,6 +44,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
> #define LOOKUP_BENEATH 0x080000 /* No escaping from starting point. */
> #define LOOKUP_IN_ROOT 0x100000 /* Treat dirfd as fs root. */
> #define LOOKUP_CACHED 0x200000 /* Only do cached lookup */
> +#define LOOKUP_DFD_MATCH_CREDS 0x400000 /* Require that dfd creds
> match current */
> /* LOOKUP_* flags which do scope-related checks based on the dirfd. */
> #define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT)
>
> --
> 2.44.0
>
> On Thu, Apr 11, 2024 at 12:44 PM Charles Mirabile <[email protected]> wrote:
> >
> > On Thu, Apr 11, 2024 at 12:15 PM Linus Torvalds
> > <[email protected]> wrote:
> > >
> > > On Thu, 11 Apr 2024 at 02:05, Christian Brauner <[email protected]> wrote:
> > > >
> > > > I had a similar discussion a while back someone requested that we relax
> > > > permissions so linkat can be used in containers.
> > >
> > > Hmm.
> > >
> > > Ok, that's different - it just wants root to be able to do it, but
> > > "root" being just in the container itself.
> > >
> > > I don't think that's all that useful - I think one of the issues with
> > > linkat(AT_EMPTY_PATH) is exactly that "it's only useful for root",
> > > which means that it's effectively useless. Inside a container or out.
> > >
> > > Because very few loads run as root-only (and fewer still run with any
> > > capability bits that aren't just "root or nothing").
> > >
> > > Before I did all this, I did a Debian code search for linkat with
> > > AT_EMPTY_PATH, and it's almost non-existent. And I think it's exactly
> > > because of this "when it's only useful for root, it's hardly useful at
> > > all" issue.
> > >
> > > (Of course, my Debian code search may have been broken).
> > >
> > > So I suspect your special case is actually largely useless, and what
> > > the container user actually wanted was what my patch does, but they
> > > didn't think that was possible, so they asked to just extend the
> > > "root" notion.
> > >
> > Yes, that is absolutely the case. When Christian poked holes in my
> > initial submission, I started working on a v2 but haven't had a chance
> > to send it because I wanted to make sure my arguments etc were
> > airtight because I am well aware of just how long and storied the
> > history of flink is. In the v2 that I didn't post yet, I extended the
> > ability to link *any* file from only true root to also "root" within a
> > container following the potentially suspect approach that christian
> > suggested (I see where you are coming from with the unobvious approach
> > to avoiding toctou though I obviously support this patch being
> > merged), but I added a followup patch that checks for whether the file
> > in question is an `__O_TMPFILE` file which is trivial once we are
> > jumping through the hoops of looking up the struct file. If it is a
> > tmpfile (i.e. linkcount = 0) I think that all the concerns raised
> > about processes that inherit a fd being able to create links to the
> > file inappropriately are moot. Here is an excerpt from the cover
> > letter I was planning to send that explains my reasoning.
> >
> > - the ability to create an unnamed file, adjust its contents
> > and attributes (i.e. uid, gid, mode, xattrs, etc) and then only give it a name
> > once they are ready is exactly the ideal use-case for a hypothetical `flink`
> > system call. The ability to use `AT_EMPTY_PATH` with `linkat` essentially turns
> > it into `flink`, but these restrictions hamper it from actually being used, as
> > most code is not privileged. By checking whether the file to be linked is a
> > temporary (i.e. unnamed) file we can allow this useful case without allowing
> > the more risky cases that could have security implications.
> >
> > - Although it might appear that the security posture is affected, it is not.
> > Callers who open an extant file on disk in read only mode for sharing with
> > potentially untrustworthy code can trust that this change will not affect them
> > because it only applies to temporary files. Callers who open a temporary file
> > need to do so with write access if they want to actually put any data in it,
> > so they will already have to reopen the file (e.g. by linking it to a path
> > and opening that path, or opening the magic symlink in proc) if they want to
> > share it in read-only mode with untrustworthy code. As such, that new file
> > description will no longer be marked as a temporary file and thus these changes
> > do not apply. The final case I can think of is the unlikely possibility of
> > intentionally sharing read write access to data stored in a temporary file with
> > untrustworthy code, but where that code being able to make a link would still
> > be deleterious. In such a bizarre case, the `O_EXCL` can and should be used to
> > fully prevent the temporary file from ever having a name, and this change does
> > not alter its efficacy.
> >
> > > I've added Charles to the Cc.
> > >
> > > But yes, with my patch, it would now be trivial to make that
> > >
> > > capable(CAP_DAC_READ_SEARCH)
> > >
> > > test also be
> > >
> > > ns_capable(f.file->f_cred->user_ns, CAP_DAC_READ_SEARCH)
> > >
> > > instead. I suspect not very many would care any more, but it does seem
> > > conceptually sensible.
> > >
> > > As to your patch - I don't like your nd->root games in that patch at
> > > all. That looks odd.
> > >
> > > Yes, it makes lookup ignore the dfd (so you avoid the TOCTOU issue),
> > > but it also makes lookup ignore "/". Which happens to be ok with an
> > > empty path, but still...
> > >
> > > So it feels to me like that patch of yours mis-uses something that is
> > > just meant for vfs_path_lookup().
> > >
> > > It may happen to work, but it smells really odd to me.
> > >
> > > Linus
> > >
On Thu, 11 Apr 2024 at 05:25, Christian Brauner <[email protected]> wrote:
>
> Btw, I think we should try to avoid putting this into path_init() and
> confine this to linkat() itself imho. The way I tried to do it was by
> presetting a root for filename_lookup(); means we also don't need a
> LOOKUP_* flag for this as this is mostly a linkat thing.
So I had the exact reverse reaction to your patch - I felt that using
that 'root' thing was the hacky case.
The lookup flag may be limited to linkat(), but it makes the code
smaller and clearer, and avoids having multiple places where we check
dfd.
And that 'root' argument really is the special hacky case, and is not
actually used by any normal system call path, and is meant for
internal kernel use rather than any generic case.
Linus
On Thu, 11 Apr 2024 at 11:13, Linus Torvalds
<[email protected]> wrote:
>
> 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.
Just to clarify: I think the ns_capable() change is a good idea and
makes sense. The whole "limited to global root" makes no sense if the
file was opened within a namespace, and I think it always just came
from the better check not being obvious at the point where
AT_EMPTY_PATH was checked for.
Similarly, while the FMODE_PATH test _looks_ very similar to an
O_TMPFILE check, I think it's fundamentally different in a conceptual
sense: not only is FMODE_PATH filesystem-agnostic, a FMODE_PATH file
is *only* useful as a pathname (ie no read/write semantics).
And so if a FMODE_PATH file descriptor is passed in from the outside,
I feel like the "you cannot use this to create a path" is kind of a
fundamentally nonsensical rule.
IOW, whoever is passing that FMODE_PATH file descriptor around must
have actually thought about it, and must have opened it with O_PATH,
and it isn't useful for anything else than as a starting point for a
path lookup.
So while I don't think the __O_TMPFILE exception would necessarily be
wrong per se, I am afraid that it would result in people writing
convenient code that "appears to work" in testing, but then fails when
run in an environment where the directory is mounted over NFS (or any
other filesystem that doesn't do ->tmpfile()).
I am certainly open to be convinced otherwise, but I really think that
the real pattern to aim for should just be "look, I opened the file
myself, then filled in the detail, and now I'm doing a linkat() to
expose it" and that the real protection issue should be that "my
credentials are the same for open and linkat".
The other rules (ie the capability check or the FMODE_PATH case) would
be literally about situations where you *want* to pass things around
between protection domains.
In that context, the ns_capable() and the FMODE_PATH check make sense to me.
In contrast, the __O_TMPFILE check just feels like a random detail.
Hmm?
Anyway, end result of that is that this is what that part of the patch
looks like for me right now:
+ if (flags & LOOKUP_DFD_MATCH_CREDS) {
+ const struct cred *cred = f.file->f_cred;
+ if (!(f.file->f_mode & FMODE_PATH) &&
+ cred != current_cred() &&
+ !ns_capable(cred->user_ns, CAP_DAC_READ_SEARCH)) {
+ fdput(f);
+ return ERR_PTR(-ENOENT);
+ }
+ }
and that _seems_ sensible to me.
But yes, this all has been something that we have failed to do right
for at least a quarter of a century so far, so this needs a *lot* of
thought, even if the patch itself is rather small and looks relatively
obvious.
Linus
On Thu, 11 Apr 2024 at 10:35, Charles Mirabile <[email protected]> wrote:
>
> 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.
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.
Linus
On Thu, Apr 11, 2024 at 2:14 PM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, 11 Apr 2024 at 10:35, Charles Mirabile <[email protected]> wrote:
> >
> > 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
>
On Thu, 11 Apr 2024 at 13:08, Charles Mirabile <[email protected]> wrote:
>
> 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.
Oh, absolutely. I think the right pattern is basically some variation of
fd = open(filename, O_TMPFILE | O_WRONLY, 0600);
if (fd < 0) {
char template{...] = ".tmpfileXXXXXX";
fd = mkstmp(template);
unlink(template);
}
.. now act on fd to initialize it ..
linkat(fd, "", AT_FDCWD, "finalname", AT_EMPTY_PATH);
which should work reasonably well in various environments.
Clearly O_TMPFILE is the superior option when it exists. I'm just
saying that anything that *relies* on it existing is dubious.
Linus
On Thu, Apr 11, 2024 at 12:44:46PM -0400, Charles Mirabile wrote:
> On Thu, Apr 11, 2024 at 12:15 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Thu, 11 Apr 2024 at 02:05, Christian Brauner <[email protected]> wrote:
> > >
> > > I had a similar discussion a while back someone requested that we relax
> > > permissions so linkat can be used in containers.
> >
> > Hmm.
> >
> > Ok, that's different - it just wants root to be able to do it, but
> > "root" being just in the container itself.
> >
> > I don't think that's all that useful - I think one of the issues with
> > linkat(AT_EMPTY_PATH) is exactly that "it's only useful for root",
> > which means that it's effectively useless. Inside a container or out.
> >
> > Because very few loads run as root-only (and fewer still run with any
> > capability bits that aren't just "root or nothing").
> >
> > Before I did all this, I did a Debian code search for linkat with
> > AT_EMPTY_PATH, and it's almost non-existent. And I think it's exactly
> > because of this "when it's only useful for root, it's hardly useful at
> > all" issue.
> >
> > (Of course, my Debian code search may have been broken).
> >
> > So I suspect your special case is actually largely useless, and what
> > the container user actually wanted was what my patch does, but they
> > didn't think that was possible, so they asked to just extend the
> > "root" notion.
> >
> Yes, that is absolutely the case. When Christian poked holes in my
> initial submission, I started working on a v2 but haven't had a chance
> to send it because I wanted to make sure my arguments etc were
> airtight because I am well aware of just how long and storied the
> history of flink is. In the v2 that I didn't post yet, I extended the
> ability to link *any* file from only true root to also "root" within a
> container following the potentially suspect approach that christian
> suggested (I see where you are coming from with the unobvious approach
I'm sorry, what is suspect about my approach? Your patch in [1] lowered
the capability checks to ns_capable(current_user_ns(), CAP_DAC_READ_SEARCH).
That's very much wrong because it means root in an unprivileged
container could linkat() any file descriptor including any opened by
real root. The permission needs to be tied to the opener of the file.
Otherwise that's guaranteed to break security assumptions. Whereas my
patch avoids that.
[1]: https://lore.kernel.org/all/[email protected]
On Thu, Apr 11, 2024 at 11:13:53AM -0700, Linus Torvalds wrote:
> On Thu, 11 Apr 2024 at 10:35, Charles Mirabile <[email protected]> wrote:
> >
> > 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.
Yes. There's no need to give O_TMPFILE special status there. We also end
up with a collection of special-cases which is just unpleasant.
On Thu, Apr 11, 2024 at 12:34:52PM -0700, Linus Torvalds wrote:
> On Thu, 11 Apr 2024 at 11:13, Linus Torvalds
> <[email protected]> wrote:
> >
> > 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.
>
> Just to clarify: I think the ns_capable() change is a good idea and
> makes sense. The whole "limited to global root" makes no sense if the
> file was opened within a namespace, and I think it always just came
> from the better check not being obvious at the point where
> AT_EMPTY_PATH was checked for.
>
> Similarly, while the FMODE_PATH test _looks_ very similar to an
> O_TMPFILE check, I think it's fundamentally different in a conceptual
> sense: not only is FMODE_PATH filesystem-agnostic, a FMODE_PATH file
> is *only* useful as a pathname (ie no read/write semantics).
But the annoying part imho is that we also allow stuff like fsetxattr()
to work on O_PATH file descriptors. Something that I really dislike.
>
> And so if a FMODE_PATH file descriptor is passed in from the outside,
> I feel like the "you cannot use this to create a path" is kind of a
> fundamentally nonsensical rule.
Hm, I would like to avoid adding an exception for O_PATH. While it is
useful for lookup O_PATH is also often used simply as an opaque handle
to a file. O_PATH is often used to simply identify or compare files, or
for execveat(). Especially for cross-container interactions it would be
nice to not generally allow O_PATH to be linkat()ed. The current
behavior proposed in this patch should be enough.
On Thu, Apr 11, 2024 at 09:21:27AM -0700, Linus Torvalds wrote:
> On Thu, 11 Apr 2024 at 05:25, Christian Brauner <[email protected]> wrote:
> >
> > Btw, I think we should try to avoid putting this into path_init() and
> > confine this to linkat() itself imho. The way I tried to do it was by
> > presetting a root for filename_lookup(); means we also don't need a
> > LOOKUP_* flag for this as this is mostly a linkat thing.
>
> So I had the exact reverse reaction to your patch - I felt that using
Fun. I really disliked the idea of having to somehow wrangle this into
lookup itself and that the root argument just happened to not be used
for non-kernel internal use-cases yet. But I'm not married to this so
fine by me.
On Wed, 10 Apr 2024 17:10:12 -0700, Linus Torvalds wrote:
> "The definition of insanity is doing the same thing over and over
> again and expecting different results”
>
> We've tried to do this before, most recently with commit bb2314b47996
> ("fs: Allow unprivileged linkat(..., AT_EMPTY_PATH) aka flink") about a
> decade ago.
>
> [...]
So it seems that this might be worth trying. I've picked up the patch
with two modifications:
(1) added the relaxed capability check.
(2) renamed the flag to LOOKUP_LINKAT_EMPTY
(3) slight adjustment to commit message
Should show up in -next if I don't hear objections or you want to apply
this directly. Fingers crossed we don't see regressions.
---
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/1] vfs: relax linkat() AT_EMPTY_PATH - aka flink() - requirements
https://git.kernel.org/vfs/vfs/c/fa75d6e377fd
On Fri, 12 Apr 2024 at 00:46, Christian Brauner <[email protected]> wrote:
>
> Hm, I would like to avoid adding an exception for O_PATH.
Ack. It's not the important or really relevant part.
Linus
Side note: I'd really like to relax another unrelated AT_EMPTY_PATH
issue: we should just allow a NULL path for that case.
The requirement that you pass an actual empty string is insane. It's
wrong. And it adds a noticeable amount of expense to this path,
because just getting the single byte and looking it up is fairly
expensive.
This was more noticeable because glibc at one point (still?) did
newfstatat(6, "", buf, AT_EMPTY_PATH)
when it should have just done a simple "fstat()".
So there were (are?) a *LOT* of AT_EMPTY_PATH users, and they all do a
pointless "let's copy a string from user space".
And yes, I know exactly why AT_EMPTY_PATH exists: because POSIX
traditionally says that a path of "" has to return -ENOENT, not the
current working directory. So AT_EMPTY_PATH basically says "allow the
empty path for lookup".
But while it *allows* the empty path, it does't *force* it, so it
doesn't mean "avoid the lookup", and we really end up doing a lot of
extra work just for this case. Just the user string copy is a big deal
because of the whole overhead of accessing user space, but it's also
the whole "allocate memory for the path etc".
If we either said "a NULL path with AT_EMPTY_PATH means empty", or
even just added a new AT_NULL_PATH thing that means "path has to be
NULL, and it means the same as AT_EMPTY_PATH with an empty path", we'd
be able to avoid quite a bit of pointless work.
Linus
this is a nitpic, but if you are going to touch the comment on
line 4654 I think you should fix the spelling mistake :^)
handlink -> hard link
Signed-off-by: Charles Mirabile <[email protected]>
---
fs/namei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 6d5b3f0d6ad3..9b806b108ed0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4651,7 +4651,7 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
* To use null names we require CAP_DAC_READ_SEARCH or
* that the open-time creds of the dfd matches current.
* This ensures that not everyone will be able to create
- * handlink using the passed file descriptor.
+ * hard links using the passed file descriptor.
*/
if (flags & AT_EMPTY_PATH)
how |= LOOKUP_LINKAT_EMPTY;
--
2.44.0
On Fri, Apr 12, 2024 at 10:43:06AM -0700, Linus Torvalds wrote:
> Side note: I'd really like to relax another unrelated AT_EMPTY_PATH
> issue: we should just allow a NULL path for that case.
>
> The requirement that you pass an actual empty string is insane. It's
> wrong. And it adds a noticeable amount of expense to this path,
> because just getting the single byte and looking it up is fairly
> expensive.
>
> This was more noticeable because glibc at one point (still?) did
>
> newfstatat(6, "", buf, AT_EMPTY_PATH)
>
> when it should have just done a simple "fstat()".
>
> So there were (are?) a *LOT* of AT_EMPTY_PATH users, and they all do a
> pointless "let's copy a string from user space".
>
> And yes, I know exactly why AT_EMPTY_PATH exists: because POSIX
> traditionally says that a path of "" has to return -ENOENT, not the
> current working directory. So AT_EMPTY_PATH basically says "allow the
> empty path for lookup".
>
> But while it *allows* the empty path, it does't *force* it, so it
> doesn't mean "avoid the lookup", and we really end up doing a lot of
> extra work just for this case. Just the user string copy is a big deal
> because of the whole overhead of accessing user space, but it's also
> the whole "allocate memory for the path etc".
>
> If we either said "a NULL path with AT_EMPTY_PATH means empty", or
> even just added a new AT_NULL_PATH thing that means "path has to be
> NULL, and it means the same as AT_EMPTY_PATH with an empty path", we'd
> be able to avoid quite a bit of pointless work.
It also causes issues for sandboxed enviroments (most recently for the
Chrome sandbox) because AT_EMPTY_PATH doesn't actually mean
AT_EMPTY_PATH unless the string is actually empty. Otherwise
AT_EMPTY_PATH is ignored. So I'm all on board for this. I need to think
a bit whether AT_NULL_PATH or just allowing NULL would be nicer. Mostly
because I want to ensure that userspace can easily detect this new
feature.
On Sat, Apr 13, 2024 at 11:41:57AM +0200, Christian Brauner wrote:
> On Fri, Apr 12, 2024 at 10:43:06AM -0700, Linus Torvalds wrote:
> > Side note: I'd really like to relax another unrelated AT_EMPTY_PATH
> > issue: we should just allow a NULL path for that case.
> >
> > The requirement that you pass an actual empty string is insane. It's
> > wrong. And it adds a noticeable amount of expense to this path,
> > because just getting the single byte and looking it up is fairly
> > expensive.
> >
> > This was more noticeable because glibc at one point (still?) did
> >
> > newfstatat(6, "", buf, AT_EMPTY_PATH)
> >
> > when it should have just done a simple "fstat()".
> >
> > So there were (are?) a *LOT* of AT_EMPTY_PATH users, and they all do a
> > pointless "let's copy a string from user space".
> >
> > And yes, I know exactly why AT_EMPTY_PATH exists: because POSIX
> > traditionally says that a path of "" has to return -ENOENT, not the
> > current working directory. So AT_EMPTY_PATH basically says "allow the
> > empty path for lookup".
> >
> > But while it *allows* the empty path, it does't *force* it, so it
> > doesn't mean "avoid the lookup", and we really end up doing a lot of
> > extra work just for this case. Just the user string copy is a big deal
> > because of the whole overhead of accessing user space, but it's also
> > the whole "allocate memory for the path etc".
> >
> > If we either said "a NULL path with AT_EMPTY_PATH means empty", or
> > even just added a new AT_NULL_PATH thing that means "path has to be
> > NULL, and it means the same as AT_EMPTY_PATH with an empty path", we'd
> > be able to avoid quite a bit of pointless work.
>
> It also causes issues for sandboxed enviroments (most recently for the
> Chrome sandbox) because AT_EMPTY_PATH doesn't actually mean
> AT_EMPTY_PATH unless the string is actually empty. Otherwise
> AT_EMPTY_PATH is ignored. So I'm all on board for this. I need to think
> a bit whether AT_NULL_PATH or just allowing NULL would be nicer. Mostly
> because I want to ensure that userspace can easily detect this new
> feature.
I think it should be ok to allow AT_EMPTY_PATH with NULL because
userspace can detect whether the kernel allows that by passing
AT_EMPTY_PATH with a NULL path argument and they would get an error back
that would tell them that this kernel doesn't support NULL paths.
I'd like to try a patch for this next week. It's a good opportunity to
get into some of the more gritty details of this area.
From a rough first glance most AT_EMPTY_PATH users should be covered by
adapting getname_flags() accordingly.
Imho, this could likely be done by introducing a single struct filename
null_filename. That also takes care of audit that reuses the pathname.
That thing would basically never go away and the refcnt remain fixed at
one. Kind of similar to what we did for struct mnt_idmap nop_mnt_idmap.
That's at least what I naively hope for but I haven't yet starting
looking into all the dark corners.
On Sat, 13 Apr 2024 at 08:16, Christian Brauner <[email protected]> wrote:
>
> I think it should be ok to allow AT_EMPTY_PATH with NULL because
> userspace can detect whether the kernel allows that by passing
> AT_EMPTY_PATH with a NULL path argument and they would get an error back
> that would tell them that this kernel doesn't support NULL paths.
Yeah, it should return -1 / EFAULT on older kernels.
> I'd like to try a patch for this next week. It's a good opportunity to
> get into some of the more gritty details of this area.
>
> From a rough first glance most AT_EMPTY_PATH users should be covered by
> adapting getname_flags() accordingly.
>
> Imho, this could likely be done by introducing a single struct filename
> null_filename.
It's probably better to try to special-case it entirely.
See commit 9013c51c630a ("vfs: mostly undo glibc turning 'fstat()'
into 'fstatat(AT_EMPTY_PATH)'") and the numbers in there in
particular.
That still leaves performance on the table exactly because it has to
do that extra "get_user()" to check for an empty path, but it avoids
not only the pathname allocation, but also the setup for the pathname
walk.
If we had a NULL case there, I'd expect that fstatat() and fstat()
would perform the same (modulo a couple of instructions).
Of course, the performance of get_user() will vary depending on
microarchitecture. If you don't have SMAP, it's cheap. It's the
STAC/CLAC that is most of the cost, and the exact cost of those will
then depend on implementations - they *could* be much faster than they
are.
Linus