Message-ID: <20240802.091810-fresh.camps.mild.skyline-lIrajRPgkI@cyphar.com>
Date: Fri, 2 Aug 2024 19:35:19 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: Andrea Cervesato <andrea.cervesato@...e.com>
Cc: Petr Vorel <pvorel@...e.cz>, 
	Andrea Cervesato <andrea.cervesato@...e.de>, ltp@...ts.linux.it, Alexey Gladkov <legion@...nel.org>, 
	Christian Brauner <brauner@...nel.org>, Cyril Hrubis <chrubis@...e.cz>, 
	Adhemerval Zanella <adhemerval.zanella@...aro.org>, Gaël PORTAY <gael.portay@...ne.fr>, 
	linux-kernel@...r.kernel.org
Subject: Re: [LTP] [PATCH v4 0/5] Add fchmodat2 testing suite

On 2024-08-02, Andrea Cervesato <andrea.cervesato@...e.com> wrote:
> Hi!
> 
> On 8/2/24 07:42, Petr Vorel wrote:
> > > On 2024-08-01, Petr Vorel <pvorel@...e.cz> wrote:
> > > > Hi all,
> > > > > This is a patch-set that implements fchmodat2() syscall coverage.
> > > > > fchmodat2() has been added in kernel 6.6 in order to support
> > > > > AT_SYMLINK_NOFOLLOW and AT_EMPTY_PATH in fchmodat().
> > > > > There's no man pages yet, so please take the following links as
> > > > > main documentation along with kernel source code:
> > > > I would hope that it'd be at least Christian's fork [1], but it's not there.
> > > > I suppose nobody is working on the man page.
> > > > > https://www.phoronix.com/news/fchmodat2-For-Linux-6.6
> > > > > https://lore.kernel.org/lkml/20230824-frohlocken-vorabend-725f6fdaad50@brauner/
> > > > > ***********
> > > > > * WARNING *
> > > > > ***********
> > > > > fchmodat2_02 fails with EOPNOTSUPP because of missing feature.
> > > > For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> > > > 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
> > > > Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> > > > LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> > > > fixed.
> > > > Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> > > > different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> > > > just accepted behavior (glibc returns EOPNOTSUPP on symlink):
> > > > +  /* Some Linux versions with some file systems can actually
> > > > +     change symbolic link permissions via /proc, but this is not
> > > > +     intentional, and it gives inconsistent results (e.g., error
> > > > +     return despite mode change).  The expected behavior is that
> > > > +     symbolic link modes cannot be changed at all, and this check
> > > > +     enforces that.  */
> > > > +  if (S_ISLNK (st.st_mode))
> > > > +    {
> > > >         __close_nocancel (pathfd);
> > > > -      return ret;
> > > > +      __set_errno (EOPNOTSUPP);
> > > > +      return -1;
> > > > +    }
> > > > Also musl also behaves the same on his fallback on old kernels [3]
> > > > (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> > > > argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> > > > year SYS_fchmodat2 started to be used in d0ed307e):
> > > > 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> > > > 	if (ret != -ENOSYS) return __syscall_ret(ret);
> > > > 	if (flag != AT_SYMLINK_NOFOLLOW)
> > > > 		return __syscall_ret(-EINVAL);
> > > > 	struct stat st;
> > > > 	int fd2;
> > > > 	char proc[15+3*sizeof(int)];
> > > > 	if (fstatat(fd, path, &st, flag))
> > > > 		return -1;
> > > > 	if (S_ISLNK(st.st_mode))
> > > > 		return __syscall_ret(-EOPNOTSUPP);
> > 
> > > > > According to documentation, the feature has been implemented in
> > > > > kernel 6.6, but __in reality__ AT_SYMLINK_NOFOLLOW is not working
> > > > > on symbolic files. Also kselftests, which are meant to test the
> > > > > functionality, are not working and they are treating fchmodat2()
> > > > > syscall failure as SKIP. Please take a look at the following code
> > > > > before reviewing:
> > > > > https://github.com/torvalds/linux/blob/8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6/tools/testing/selftests/fchmodat2/fchmodat2_test.c#L123
> > > > I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> > > > selftest") [4], where fchmodat2 failure on symlink is simply skipped.
> > > > Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> > > > anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> > > > bugs, if it's not expected to be working we might just skip the test as well.
> > > If I understand the bug report, the issue is that fchmodat2() doesn't
> > > work on symlinks?
> > Yes.
> > 
> > > This is intentional -- Christian fixed a tree-wide bug a while ago[1]
> > > where some filesystems would change the mode of symlinks despite
> > > returning an error (usually EOPNOTSUPP) and IIRC a few others would
> > > happily change the mode of symlinks.
> > Ah, I've seen this in the past. Thanks a lot for reminding me.
> > 
> > > The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
> > > there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
> > > was chosen because that's what filesystems were already returning.
> > > (While this is a little confusing, VFS syscalls return EINVAL for an
> > > unsupported flag, not EOPNOTSUPP.)
> > > The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
> > > syscall to operate on symlinks, it also allows programs to safely
> > > operate on path components without worrying about symlinks being
> > > followed (this is relevant for container runtimes, where we are
> > > operating on untrusted filesystem roots -- though in the case of
> > > fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
> > > an error here is actually what you want as a program that uses
> > > AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
> > > supported by filesystems).
>
> Thanks for the explanation. I also have a question around this topic:
> AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being
> followed. [...] Also, is it an unsupported feature only on certain
> filesystems?

AFAIK it was never supposed to be supported by filesystems, there was a
series of bugs that lead to it working by accident and that has now been
fixed. It's blocked by the VFS now, so no filesystems support it.

EOPNOTSUPP was chosen as the error code to avoid breaking userspace
because that was the error code used by glibc as well as some
filesystems. I wouldn't interpret this EOPNOTSUPP as meaning "we plan to
add support for this in the future" nor "only some filesystems don't
support this" -- I would just treat it if it were ELOOP.

> But if filesystems are not supporting it, why do we have this feature?

The problem being solved is the same as O_NOFOLLOW. Before O_PATH,
O_NOFOLLOW would always return an error -- this was what you wanted
because you wanted to open a file without following (trailing) symlinks.
If the final component was a symlink you wanted to get an error rather
than following the symlink and opening some other file (this could be a
particular problem if you are dealing with an extracted rootfs tree --
symlinks could escape to the host and you could end up operating on host
files). These kinds of problems crop up a lot when dealing with
privileged tools that need to deal with untrusted directory trees. (If
you're interested, I'm working on a path resolution library that makes
use of these kinds of tricks[1].)

That being said, O_NOFOLLOW/AT_SYMLINK_NOFOLLOW doesn't help you with
symlink path components. That's what openat2(RESOLVE_IN_ROOT) or
openat2(RESOLVE_NO_SYMLINKS) is for. So in most cases, I suspect people
are going to want to use openat2+fchmodat2(AT_EMPTY_PATH) instead but
AT_SYMLINK_NOFOLLOW does still make sense for this kind of syscall.

Modern VFS syscalls are designed to make sure that it's possible for you
to either operate on a file descriptor (AT_EMPTY_PATH) or to ensure
trailing symlinks are not followed (AT_SYMLINK_NOFOLLOW). This lets you
make sure that you are never operating on a path that could change
underneath you. With AT_SYMLINK_NOFOLLOW as long as the path doesn't
contain "/" you can safely operate on any path (you just need to first
open the parent directory, either with openat2(RESOLVE_*) or by opening
each component of the path using openat[2,3]).

[1]: https://github.com/openSUSE/libpathrs
[2]: https://github.com/cyphar/filepath-securejoin/blob/v0.3.1/lookup_linux.go#L178
[3]: https://github.com/openSUSE/libpathrs/blob/de588611aa9a/src/resolvers/opath.rs

> > Thanks a lot for explaining the background!
> > 
> > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
> > > > I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> > > > but AFAIK it's not related to this problem.
> > > Yeah this is unrelated, that patch is about clarifying how AT_* flags
> > > are allocated, not syscall behaviour.
> > Thanks!
> > 
> > > > Kind regards,
> > > > Petr
> > @Andrea, I guess we want something like this:
> > 
> > +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> > @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
> >   	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> >   	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
> > -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> > -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> > -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> > +	if (tst_kvercmp(6, 6, 0) >= 0) {
> > +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> > +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> > +	}
> >   }
> >   static void test_empty_folder(void)
> 
> I think it makes more sense to filter out only filesystems which are not
> supporting this feature (see my comment above).
> 
> Best regards,
> Andrea
> 

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists