Re: [RFC][PATCHSET] sanitized pathwalk machinery (v2)

[Aleksa Sarai]

On 2020-02-25, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Sun, Feb 23, 2020 at 01:12:21AM +0000, Al Viro wrote:
> > 	This is a slightly extended repost of the patchset posted on
> > Jan 19.  Current branch is in vfs.git#work.do_last, the main
> > difference from the last time around being a bit of do_last()
> > untangling added in the end of series.  #work.openat2 is already
> > in mainline, which simplifies the series - now it's a straight
> > branch with no merges.
> 
> Whee...  While trying to massage ".." handling towards the use of
> regular mount crossing semantics, I've found something interesting.
> Namely, if you start in a directory with overmounted parent,
> LOOKUP_NO_XDEV resolution of ../something will bloody well cross
> into the overmount.

Oh boy...

> Reason: follow_dotdot() (and its RCU counterpart) check for LOOKUP_NO_XDEV
> when crossing into underlying fs, but not when crossing into overmount
> of the parent.
> 
> Interpretation of .. is basically
> 
> loop:	if we are in root					// uncommon
> 		next = current position
> 	else if we are in root of a mounted filesystem		// more rare
> 		move to underlying mountpoint
> 		goto loop
> 	else
> 		next = parent directory of current position	// most common
> 
> 	while next is overmounted				// _VERY_ uncommon
> 		next = whatever's mounted on next
> 
> 	move to next
> 
> The second loop should've been sharing code with the normal mountpoint
> crossing.  It doesn't, which has already lead to interesting inconsistencies
> (e.g. autofs generally expects ->d_manage() to be called before crossing
> into it; here it's not done).  LOOKUP_NO_XDEV has just added one more...

You're quite right -- LOOKUP_NO_XDEV should block that and I missed it.

> Incidentally, another inconsistency is LOOKUP_BENEATH treatment in case
> when we have walked out of the subtree by way of e.g. procfs symlink and
> then ran into .. in the absolute root (that's
>                 if (!follow_up(&nd->path))
>                         break;
> in follow_dotdot()).  Shouldn't that give the same reaction as ..
> in root (EXDEV on LOOKUP_BENEATH, that is)?  It doesn't...

You can't go through procfs symlinks with LOOKUP_BENEATH, but if it's
possible to do that kind of jump then it should also be blocked (but I
would say that I'd prefer "block any kind of weird jump").

> Another one is about LOOKUP_NO_XDEV again: suppose you have process'
> root directly overmounted and cwd in the root of whatever's overmounting
> it.  Resolution of .. will stay in cwd - we have no parent within the
> chroot jail we are in, so we move to whatever's overmounting that root.
> Which is the original location.  Should we fail on LOOKUP_NO_XDEV here?
> Plain .. in the root of chroot jail (not overmounted by anything) does
> *not*...

I think LOOKUP_NO_XDEV should block that since you end up crossing a
mountpoint.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachment: signature.asc
Description: PGP signature