Re: [PATCH v16 06/12] namei: LOOKUP_NO_XDEV: block mountpoint crossing

[Aleksa Sarai]

On 2019-11-16, Al Viro  wrote:
> On Sat, Nov 16, 2019 at 11:27:56AM +1100, Aleksa Sarai wrote:
> 
> > @@ -1383,6 +1398,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > return -ECHILD;
> > if (>mnt == nd->path.mnt)
> > break;
> > +   if (unlikely(nd->flags & LOOKUP_NO_XDEV))
> > +   return -EXDEV;
> > /* we know that mountpoint was pinned */
> > nd->path.dentry = mountpoint;
> > nd->path.mnt = >mnt;
> > @@ -1397,6 +1414,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > return -ECHILD;
> > if (!mounted)
> > break;
> > +   if (unlikely(nd->flags & LOOKUP_NO_XDEV))
> > +   return -EXDEV;
> > nd->path.mnt = >mnt;
> > nd->path.dentry = mounted->mnt.mnt_root;
> > inode = nd->path.dentry->d_inode;
> 
> I really don't think we should return hard errors from that function.
> Let the caller redo it in refwalk mode.

I suspected as much, though my reason for not changing it was that the
mount_lock check should ensure that the cached status of whether ".." is
a mountpoint crossing is correct. But I guess this is more about being
safe than sorry, rather than an actual bug?

> It's not the fast path, especially for this kind of errors.  Matter of
> fact, I'm not sure about -ENOENT returned in another failure case
> there - it's probably OK, but again, -ECHILD would be just as good.

I can switch the -ENOENT too if you like.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH



signature.asc
Description: PGP signature

Re: [PATCH v16 06/12] namei: LOOKUP_NO_XDEV: block mountpoint crossing

[Al Viro]

On Sat, Nov 16, 2019 at 11:27:56AM +1100, Aleksa Sarai wrote:

> @@ -1383,6 +1398,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
>   return -ECHILD;
>   if (>mnt == nd->path.mnt)
>   break;
> + if (unlikely(nd->flags & LOOKUP_NO_XDEV))
> + return -EXDEV;
>   /* we know that mountpoint was pinned */
>   nd->path.dentry = mountpoint;
>   nd->path.mnt = >mnt;
> @@ -1397,6 +1414,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
>   return -ECHILD;
>   if (!mounted)
>   break;
> + if (unlikely(nd->flags & LOOKUP_NO_XDEV))
> + return -EXDEV;
>   nd->path.mnt = >mnt;
>   nd->path.dentry = mounted->mnt.mnt_root;
>   inode = nd->path.dentry->d_inode;

I really don't think we should return hard errors from that function.
Let the caller redo it in refwalk mode.

It's not the fast path, especially for this kind of errors.  Matter of
fact, I'm not sure about -ENOENT returned in another failure case
there - it's probably OK, but again, -ECHILD would be just as good.

[PATCH v16 06/12] namei: LOOKUP_NO_XDEV: block mountpoint crossing

[Aleksa Sarai]

/* Background. */
The need to contain path operations within a mountpoint has been a
long-standing usecase that userspace has historically implemented
manually with liberal usage of stat(). find, rsync, tar and
many other programs implement these semantics -- but it'd be much
simpler to have a fool-proof way of refusing to open a path if it
crosses a mountpoint.

This is part of a refresh of Al's AT_NO_JUMPS patchset[1] (which was a
variation on David Drysdale's O_BENEATH patchset[2], which in turn was
based on the Capsicum project[3]).

/* Userspace API. */
LOOKUP_NO_XDEV will be exposed to userspace through openat2(2).

/* Semantics. */
Unlike most other LOOKUP flags (most notably LOOKUP_FOLLOW),
LOOKUP_NO_XDEV applies to all components of the path.

With LOOKUP_NO_XDEV, any path component which crosses a mount-point
during path resolution (including "..") will yield an -EXDEV. Absolute
paths, absolute symlinks, and magic-links will only yield an -EXDEV if
the jump involved changing mount-points.

/* Testing. */
LOOKUP_NO_XDEV is tested as part of the openat2(2) selftests.

[1]: https://lore.kernel.org/lkml/20170429220414.gt29...@zeniv.linux.org.uk/
[2]: 
https://lore.kernel.org/lkml/1415094884-18349-1-git-send-email-drysd...@google.com/
[3]: 
https://lore.kernel.org/lkml/1404124096-21445-1-git-send-email-drysd...@google.com/

Cc: Christian Brauner 
Suggested-by: David Drysdale 
Suggested-by: Al Viro 
Suggested-by: Andy Lutomirski 
Suggested-by: Linus Torvalds 
Signed-off-by: Aleksa Sarai 
---
 fs/namei.c| 29 +
 include/linux/namei.h |  1 +
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index a97facc232af..854a1cbbe7b0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -838,6 +838,11 @@ static inline void path_to_nameidata(const struct path 
*path,
 
 static int nd_jump_root(struct nameidata *nd)
 {
+   if (unlikely(nd->flags & LOOKUP_NO_XDEV)) {
+   /* Absolute path arguments to path_init() are allowed. */
+   if (nd->path.mnt != NULL && nd->path.mnt != nd->root.mnt)
+   return -EXDEV;
+   }
if (!nd->root.mnt) {
int error = set_root(nd);
if (error)
@@ -871,6 +876,10 @@ int nd_jump_link(struct path *path)
 
if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
return -ELOOP;
+   if (unlikely(nd->flags & LOOKUP_NO_XDEV)) {
+   if (nd->path.mnt != path->mnt)
+   return -EXDEV;
+   }
 
path_put(>path);
nd->path = *path;
@@ -1275,12 +1284,16 @@ static int follow_managed(struct path *path, struct 
nameidata *nd)
break;
}
 
-   if (need_mntput && path->mnt == mnt)
-   mntput(path->mnt);
+   if (need_mntput) {
+   if (path->mnt == mnt)
+   mntput(path->mnt);
+   if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+   ret = -EXDEV;
+   else
+   nd->flags |= LOOKUP_JUMPED;
+   }
if (ret == -EISDIR || !ret)
ret = 1;
-   if (need_mntput)
-   nd->flags |= LOOKUP_JUMPED;
if (unlikely(ret < 0))
path_put_conditional(path, nd);
return ret;
@@ -1337,6 +1350,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, 
struct path *path,
mounted = __lookup_mnt(path->mnt, path->dentry);
if (!mounted)
break;
+   if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+   return false;
path->mnt = >mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1383,6 +1398,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (>mnt == nd->path.mnt)
break;
+   if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+   return -EXDEV;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = >mnt;
@@ -1397,6 +1414,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+   if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+   return -EXDEV;
nd->path.mnt = >mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1495,6 +1514,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(>path))
break;
+   if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+   return -EXDEV;
}
follow_mount(>path);
nd->inode =