| Message ID | 20190717012719.5524-3-palmer@sifive.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2,1/4] Non-functional cleanup of a "__user * filename" | expand |
On Tue, Jul 16, 2019 at 06:27:17PM -0700, Palmer Dabbelt wrote: > -int do_fchmodat(int dfd, const char __user *filename, umode_t mode) > +int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, int flags) > { > struct path path; > int error; > - unsigned int lookup_flags = LOOKUP_FOLLOW; > + unsigned int lookup_flags; > + > + if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW)) > + return -EINVAL; > + > + lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW; > + Why not do that in sys_fchmodat4() itself, passing lookup_flags to do_fchmodat() and updating old callers to pass it 0 as extra argument?
On Tue, 16 Jul 2019 18:48:02 PDT (-0700), viro@zeniv.linux.org.uk wrote: > On Tue, Jul 16, 2019 at 06:27:17PM -0700, Palmer Dabbelt wrote: > >> -int do_fchmodat(int dfd, const char __user *filename, umode_t mode) >> +int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, int flags) >> { >> struct path path; >> int error; >> - unsigned int lookup_flags = LOOKUP_FOLLOW; >> + unsigned int lookup_flags; >> + >> + if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW)) >> + return -EINVAL; >> + >> + lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW; >> + > > Why not do that in sys_fchmodat4() itself, passing lookup_flags to > do_fchmodat() and updating old callers to pass it 0 as extra argument? Ya, that seems better -- passing LOOKUP_FOLLOW instead of 0, to keep the behavior the same. That way I could avoid the overhead of these checks for the old syscalls, as we know they're not necessary. I'll replace this patch with the following for a v3 diff --git a/fs/open.c b/fs/open.c index b5b80469b93d..a5f99408af11 100644 --- a/fs/open.c +++ b/fs/open.c @@ -569,11 +569,12 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) return ksys_fchmod(fd, mode); } -int do_fchmodat(int dfd, const char __user *filename, umode_t mode) +int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, + int lookup_flags) { struct path path; int error; - unsigned int lookup_flags = LOOKUP_FOLLOW; + retry: error = user_path_at(dfd, filename, lookup_flags, &path); if (!error) { @@ -587,15 +588,28 @@ int do_fchmodat(int dfd, const char __user *filename, umode_t mode) return error; } +SYSCALL_DEFINE4(fchmodat4, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ + unsigned int lookup_flags; + + if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW)) + return -EINVAL; + + lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW; + + return do_fchmodat4(dfd, filename, mode, lookup_flags); +} + SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, umode_t, mode) { - return do_fchmodat(dfd, filename, mode); + return do_fchmodat4(dfd, filename, mode, LOOKUP_FOLLOW); } SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode) { - return do_fchmodat(AT_FDCWD, filename, mode); + return do_fchmodat4(AT_FDCWD, filename, mode, LOOKUP_FOLLOW); } static int chown_common(const struct path *path, uid_t user, gid_t group) diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index e1c20f1d0525..6676b1cc5485 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -81,6 +81,7 @@ struct io_uring_params; #include <linux/quota.h> #include <linux/key.h> #include <linux/personality.h> +#include <linux/namei.h> #include <trace/syscall.h> #ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER @@ -433,6 +434,8 @@ asmlinkage long sys_chroot(const char __user *filename); asmlinkage long sys_fchmod(unsigned int fd, umode_t mode); asmlinkage long sys_fchmodat(int dfd, const char __user *filename, umode_t mode); +asmlinkage long sys_fchmodat4(int dfd, const char __user *filename, + umode_t mode, int flags); asmlinkage long sys_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group, int flag); asmlinkage long sys_fchown(unsigned int fd, uid_t user, gid_t group); @@ -1320,11 +1323,12 @@ static inline long ksys_link(const char __user *oldname, return do_linkat(AT_FDCWD, oldname, AT_FDCWD, newname, 0); } -extern int do_fchmodat(int dfd, const char __user *filename, umode_t mode); +extern int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, + int flags); static inline int ksys_chmod(const char __user *filename, umode_t mode) { - return do_fchmodat(AT_FDCWD, filename, mode); + return do_fchmodat4(AT_FDCWD, filename, mode, LOOKUP_FOLLOW); } extern long do_faccessat(int dfd, const char __user *filename, int mode);
On Tue, Jul 16, 2019 at 06:27:17PM -0700, Palmer Dabbelt wrote: > man 3p says that fchmodat() takes a flags argument, but the Linux > syscall does not. There doesn't appear to be a good userspace > workaround for this issue but the implementation in the kernel is pretty > straight-forward. The specific use case where the missing flags came up > was WRT a fuse filesystem implemenation, but the functionality is pretty > generic so I'm assuming there would be other use cases. Note that we do have a workaround in musl libc with O_PATH and /proc/self/fd, but a syscall that allows a proper fix with the ugly workaround only in the fallback path for old kernels will be much appreciated! What about also doing a new SYS_faccessat4 with working AT_EACCESS flag? The workaround we have to do for it is far worse. Rich
On Tue, Jul 16, 2019 at 10:40:46PM -0400, Rich Felker wrote: > On Tue, Jul 16, 2019 at 06:27:17PM -0700, Palmer Dabbelt wrote: > > man 3p says that fchmodat() takes a flags argument, but the Linux > > syscall does not. There doesn't appear to be a good userspace > > workaround for this issue but the implementation in the kernel is pretty > > straight-forward. The specific use case where the missing flags came up > > was WRT a fuse filesystem implemenation, but the functionality is pretty > > generic so I'm assuming there would be other use cases. > > Note that we do have a workaround in musl libc with O_PATH and > /proc/self/fd, but a syscall that allows a proper fix with the ugly > workaround only in the fallback path for old kernels will be much > appreciated! > > What about also doing a new SYS_faccessat4 with working AT_EACCESS > flag? The workaround we have to do for it is far worse. Umm... That's doable, but getting into the "don't switch creds unless needed" territory. I'll need to play with that a bit and see what gives a tolerable variant... What of this part wrt AT_EACCESS? if (!issecure(SECURE_NO_SETUID_FIXUP)) { /* Clear the capabilities if we switch to a non-root user */ kuid_t root_uid = make_kuid(override_cred->user_ns, 0); if (!uid_eq(override_cred->uid, root_uid)) cap_clear(override_cred->cap_effective); else override_cred->cap_effective = override_cred->cap_permitted; }
diff --git a/fs/open.c b/fs/open.c index b5b80469b93d..2f72b4d6a2c1 100644 --- a/fs/open.c +++ b/fs/open.c @@ -569,11 +569,17 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) return ksys_fchmod(fd, mode); } -int do_fchmodat(int dfd, const char __user *filename, umode_t mode) +int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, int flags) { struct path path; int error; - unsigned int lookup_flags = LOOKUP_FOLLOW; + unsigned int lookup_flags; + + if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW)) + return -EINVAL; + + lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW; + retry: error = user_path_at(dfd, filename, lookup_flags, &path); if (!error) { @@ -587,15 +593,21 @@ int do_fchmodat(int dfd, const char __user *filename, umode_t mode) return error; } +SYSCALL_DEFINE4(fchmodat4, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ + return do_fchmodat4(dfd, filename, mode, flags); +} + SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, umode_t, mode) { - return do_fchmodat(dfd, filename, mode); + return do_fchmodat4(dfd, filename, mode, 0); } SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode) { - return do_fchmodat(AT_FDCWD, filename, mode); + return do_fchmodat4(AT_FDCWD, filename, mode, 0); } static int chown_common(const struct path *path, uid_t user, gid_t group) diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index e1c20f1d0525..a4bde25ad264 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -433,6 +433,8 @@ asmlinkage long sys_chroot(const char __user *filename); asmlinkage long sys_fchmod(unsigned int fd, umode_t mode); asmlinkage long sys_fchmodat(int dfd, const char __user *filename, umode_t mode); +asmlinkage long sys_fchmodat4(int dfd, const char __user *filename, + umode_t mode, int flags); asmlinkage long sys_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group, int flag); asmlinkage long sys_fchown(unsigned int fd, uid_t user, gid_t group); @@ -1320,11 +1322,12 @@ static inline long ksys_link(const char __user *oldname, return do_linkat(AT_FDCWD, oldname, AT_FDCWD, newname, 0); } -extern int do_fchmodat(int dfd, const char __user *filename, umode_t mode); +extern int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, + int flags); static inline int ksys_chmod(const char __user *filename, umode_t mode) { - return do_fchmodat(AT_FDCWD, filename, mode); + return do_fchmodat4(AT_FDCWD, filename, mode, 0); } extern long do_faccessat(int dfd, const char __user *filename, int mode);
man 3p says that fchmodat() takes a flags argument, but the Linux syscall does not. There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward. The specific use case where the missing flags came up was WRT a fuse filesystem implemenation, but the functionality is pretty generic so I'm assuming there would be other use cases. Signed-off-by: Palmer Dabbelt <palmer@sifive.com> --- fs/open.c | 20 ++++++++++++++++---- include/linux/syscalls.h | 7 +++++-- 2 files changed, 21 insertions(+), 6 deletions(-)