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

Aleksa Sarai
2020-02-28T01:25:06Z

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]

On 2020-02-25, Al Viro <viro@zeniv.linux.org.uk> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]