Date: Wed, 10 Apr 2019 21:10:22 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r346101 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs Message-ID: <201904102110.x3ALAMBo068004@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Wed Apr 10 21:10:21 2019 New Revision: 346101 URL: https://svnweb.freebsd.org/changeset/base/346101 Log: fusefs: various cleanups * Eliminate fuse_access_param. Whatever it was supposed to do, it seems like it was never complete. The only real function it ever seems to have had was a minor performance optimization, which I've already eliminated. * Make extended attribute operations obey the allow_other mount option. * Allow unprivileged access to the SYSTEM extattr namespace when -o default_permissions is not in use. * Disallow setextattr and deleteextattr on read-only mounts. * Add tests for a few more error cases. Sponsored by: The FreeBSD Foundation Modified: projects/fuse2/sys/fs/fuse/fuse_internal.c projects/fuse2/sys/fs/fuse/fuse_internal.h projects/fuse2/sys/fs/fuse/fuse_vnops.c projects/fuse2/tests/sys/fs/fusefs/allow_other.cc projects/fuse2/tests/sys/fs/fusefs/lookup.cc projects/fuse2/tests/sys/fs/fusefs/setattr.cc projects/fuse2/tests/sys/fs/fusefs/xattr.cc Modified: projects/fuse2/sys/fs/fuse/fuse_internal.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_internal.c Wed Apr 10 20:44:54 2019 (r346100) +++ projects/fuse2/sys/fs/fuse/fuse_internal.c Wed Apr 10 21:10:21 2019 (r346101) @@ -110,7 +110,6 @@ static int isbzero(void *buf, size_t len); int fuse_internal_access(struct vnode *vp, accmode_t mode, - struct fuse_access_param *facp, struct thread *td, struct ucred *cred) { @@ -143,13 +142,9 @@ fuse_internal_access(struct vnode *vp, } /* Unless explicitly permitted, deny everyone except the fs owner. */ - if (!(facp->facc_flags)) { - if (!(dataflags & FSESS_DAEMON_CAN_SPY)) { - int denied = fuse_match_cred(data->daemoncred, cred); - - if (denied) - return EPERM; - } + if (!(dataflags & FSESS_DAEMON_CAN_SPY)) { + if (fuse_match_cred(data->daemoncred, cred)) + return EPERM; } if (dataflags & FSESS_DEFAULT_PERMISSIONS) { Modified: projects/fuse2/sys/fs/fuse/fuse_internal.h ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_internal.h Wed Apr 10 20:44:54 2019 (r346100) +++ projects/fuse2/sys/fs/fuse/fuse_internal.h Wed Apr 10 21:10:21 2019 (r346101) @@ -197,27 +197,6 @@ fuse_validity_2_timespec(const struct fuse_entry_out * /* access */ - -#define FVP_ACCESS_NOOP 0x01 - -#define FACCESS_VA_VALID 0x01 -/* - * Caller must be the directory's owner, or the superuser, or the sticky bit - * must not be set - */ -#define FACCESS_STICKY 0x04 -/* Caller requires access to change file's owner */ -#define FACCESS_CHOWN 0x08 -#define FACCESS_SETGID 0x12 - -#define FACCESS_XQUERIES (FACCESS_STICKY | FACCESS_CHOWN | FACCESS_SETGID) - -struct fuse_access_param { - uid_t xuid; - gid_t xgid; - uint32_t facc_flags; -}; - static inline int fuse_match_cred(struct ucred *basecred, struct ucred *usercred) { @@ -233,7 +212,7 @@ fuse_match_cred(struct ucred *basecred, struct ucred * } int fuse_internal_access(struct vnode *vp, accmode_t mode, - struct fuse_access_param *facp, struct thread *td, struct ucred *cred); + struct thread *td, struct ucred *cred); /* attributes */ void fuse_internal_cache_attrs(struct vnode *vp, struct fuse_attr *attr, Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_vnops.c Wed Apr 10 20:44:54 2019 (r346100) +++ projects/fuse2/sys/fs/fuse/fuse_vnops.c Wed Apr 10 21:10:21 2019 (r346101) @@ -211,6 +211,37 @@ uma_zone_t fuse_pbuf_zone; #define fuse_vm_page_lock_queues() ((void)0) #define fuse_vm_page_unlock_queues() ((void)0) +/* Check permission for extattr operations, much like extattr_check_cred */ +static int +fuse_extattr_check_cred(struct vnode *vp, int ns, struct ucred *cred, + struct thread *td, accmode_t accmode) +{ + struct mount *mp = vnode_mount(vp); + struct fuse_data *data = fuse_get_mpdata(mp); + + /* + * Kernel-invoked always succeeds. + */ + if (cred == NOCRED) + return (0); + + /* + * Do not allow privileged processes in jail to directly manipulate + * system attributes. + */ + switch (ns) { + case EXTATTR_NAMESPACE_SYSTEM: + if (data->dataflags & FSESS_DEFAULT_PERMISSIONS) { + return (priv_check_cred(cred, PRIV_VFS_EXTATTR_SYSTEM)); + } + /* FALLTHROUGH */ + case EXTATTR_NAMESPACE_USER: + return (fuse_internal_access(vp, accmode, td, cred)); + default: + return (EPERM); + } +} + /* Get a filehandle for a directory */ static int fuse_filehandle_get_dir(struct vnode *vp, struct fuse_filehandle **fufhp, @@ -272,7 +303,6 @@ fuse_vnop_access(struct vop_access_args *ap) int accmode = ap->a_accmode; struct ucred *cred = ap->a_cred; - struct fuse_access_param facp; struct fuse_data *data = fuse_get_mpdata(vnode_mount(vp)); int err; @@ -295,9 +325,8 @@ fuse_vnop_access(struct vop_access_args *ap) if (vnode_islnk(vp)) { return 0; } - bzero(&facp, sizeof(facp)); - err = fuse_internal_access(vp, accmode, &facp, ap->a_td, ap->a_cred); + err = fuse_internal_access(vp, accmode, ap->a_td, ap->a_cred); return err; } @@ -711,7 +740,6 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) struct fuse_attr *fattr = NULL; uint64_t nid; - struct fuse_access_param facp; if (fuse_isdeadfs(dvp)) { *vpp = NULL; @@ -730,9 +758,9 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) * See further comments at further access checks. */ - bzero(&facp, sizeof(facp)); + /* TODO: consider eliminating this. Is there any good reason for it? */ if (vnode_isvroot(dvp)) { /* early permission check hack */ - if ((err = fuse_internal_access(dvp, VEXEC, &facp, td, cred))) { + if ((err = fuse_internal_access(dvp, VEXEC, td, cred))) { return err; } } @@ -831,8 +859,7 @@ calldaemon: if (lookup_err) { /* Entry not found */ if ((nameiop == CREATE || nameiop == RENAME) && islastcn) { - err = fuse_internal_access(dvp, VWRITE, &facp, td, - cred); + err = fuse_internal_access(dvp, VWRITE, td, cred); if (err) goto out; @@ -861,11 +888,8 @@ calldaemon: fattr = &(feo->attr); } - /* TODO: check for ENOTDIR */ - if ((nameiop == DELETE || nameiop == RENAME) && islastcn) { - err = fuse_internal_access(dvp, VWRITE, &facp, - td, cred); + err = fuse_internal_access(dvp, VWRITE, td, cred); if (err != 0) goto out; /* @@ -878,10 +902,8 @@ calldaemon: struct vattr dvattr; fuse_internal_getattr(dvp, &dvattr, cred, td); if ((dvattr.va_mode & S_ISTXT) && - fuse_internal_access(dvp, VADMIN, &facp, - cnp->cn_thread, cnp->cn_cred) && - fuse_internal_access(*vpp, VADMIN, &facp, - cnp->cn_thread, cnp->cn_cred)) { + fuse_internal_access(dvp, VADMIN, td, cred) && + fuse_internal_access(*vpp, VADMIN, td, cred)) { err = EPERM; goto out; } @@ -933,10 +955,6 @@ calldaemon: if (err) { goto out; } - /*err = fuse_lookup_check(dvp, vp, &facp, td, cred,*/ - /*nameiop, islastcn);*/ - /*if (err)*/ - /*goto out;*/ *vpp = vp; /* * Save the name for use in VOP_RENAME later. @@ -1088,15 +1106,12 @@ out: */ int tmpvtype = vnode_vtype(*vpp); - bzero(&facp, sizeof(facp)); - /*the early perm check hack */ - facp.facc_flags |= FACCESS_VA_VALID; - if ((tmpvtype != VDIR) && (tmpvtype != VLNK)) { err = ENOTDIR; } if (!err && !vnode_mountedhere(*vpp)) { - err = fuse_internal_access(*vpp, VEXEC, &facp, td, cred); + err = fuse_internal_access(*vpp, VEXEC, + td, cred); } if (err) { if (tmpvtype == VLNK) @@ -1528,7 +1543,6 @@ fuse_vnop_setattr(struct vop_setattr_args *ap) struct thread *td = curthread; struct fuse_dispatcher fdi; struct fuse_setattr_in *fsai; - struct fuse_access_param facp; pid_t pid = td->td_proc->p_pid; int err = 0; @@ -1545,19 +1559,12 @@ fuse_vnop_setattr(struct vop_setattr_args *ap) fsai = fdi.indata; fsai->valid = 0; - bzero(&facp, sizeof(facp)); - - facp.xuid = vap->va_uid; - facp.xgid = vap->va_gid; - if (vap->va_uid != (uid_t)VNOVAL) { - facp.facc_flags |= FACCESS_CHOWN; fsai->uid = vap->va_uid; fsai->valid |= FATTR_UID; accmode |= VADMIN; } if (vap->va_gid != (gid_t)VNOVAL) { - facp.facc_flags |= FACCESS_CHOWN; fsai->gid = vap->va_gid; fsai->valid |= FATTR_GID; accmode |= VADMIN; @@ -1615,7 +1622,7 @@ fuse_vnop_setattr(struct vop_setattr_args *ap) err = EROFS; goto out; } - err = fuse_internal_access(vp, accmode, &facp, td, cred); + err = fuse_internal_access(vp, accmode, td, cred); if (err) goto out; @@ -2028,7 +2035,7 @@ fuse_vnop_getextattr(struct vop_getextattr_args *ap) if (fuse_isdeadfs(vp)) return (ENXIO); - err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD); + err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD); if (err) return err; @@ -2109,11 +2116,15 @@ fuse_vnop_setextattr(struct vop_setextattr_args *ap) if (fuse_isdeadfs(vp)) return (ENXIO); + if (vfs_isrdonly(mp)) + return EROFS; + /* Deleting xattrs must use VOP_DELETEEXTATTR instead */ if (ap->a_uio == NULL) return (EINVAL); - err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VWRITE); + err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td, + VWRITE); if (err) return err; @@ -2244,7 +2255,7 @@ fuse_vnop_listextattr(struct vop_listextattr_args *ap) if (fuse_isdeadfs(vp)) return (ENXIO); - err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD); + err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD); if (err) return err; @@ -2352,7 +2363,11 @@ fuse_vnop_deleteextattr(struct vop_deleteextattr_args if (fuse_isdeadfs(vp)) return (ENXIO); - err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VWRITE); + if (vfs_isrdonly(mp)) + return EROFS; + + err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td, + VWRITE); if (err) return err; Modified: projects/fuse2/tests/sys/fs/fusefs/allow_other.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/allow_other.cc Wed Apr 10 20:44:54 2019 (r346100) +++ projects/fuse2/tests/sys/fs/fusefs/allow_other.cc Wed Apr 10 21:10:21 2019 (r346101) @@ -35,6 +35,7 @@ extern "C" { #include <sys/types.h> +#include <sys/extattr.h> #include <fcntl.h> #include <unistd.h> } @@ -212,6 +213,52 @@ TEST_F(NoAllowOther, disallowed_beneath_root) fd = openat(dfd, RELPATH2, O_RDONLY); if (fd >= 0) { fprintf(stderr, "openat should've failed\n"); + return(1); + } else if (errno != EPERM) { + fprintf(stderr, "Unexpected error: %s\n", + strerror(errno)); + return(1); + } + return 0; + } + ); +} + +/* + * Provide coverage for the extattr methods, which have a slightly different + * code path + */ +TEST_F(NoAllowOther, setextattr) +{ + int ino = 42; + + fork(true, [&] { + EXPECT_LOOKUP(1, RELPATH) + .WillOnce(Invoke( + ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, entry); + out->body.entry.attr_valid = UINT64_MAX; + out->body.entry.entry_valid = UINT64_MAX; + out->body.entry.attr.mode = S_IFREG | 0644; + out->body.entry.nodeid = ino; + }))); + + /* + * lookup the file to get it into the cache. + * Otherwise, the unprivileged lookup will fail with + * EACCES + */ + ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno); + }, [&]() { + const char value[] = "whatever"; + ssize_t value_len = strlen(value) + 1; + int ns = EXTATTR_NAMESPACE_USER; + ssize_t r; + + r = extattr_set_file(FULLPATH, ns, "foo", + (void*)value, value_len); + if (r >= 0) { + fprintf(stderr, "should've failed\n"); return(1); } else if (errno != EPERM) { fprintf(stderr, "Unexpected error: %s\n", Modified: projects/fuse2/tests/sys/fs/fusefs/lookup.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/lookup.cc Wed Apr 10 20:44:54 2019 (r346100) +++ projects/fuse2/tests/sys/fs/fusefs/lookup.cc Wed Apr 10 21:10:21 2019 (r346101) @@ -144,8 +144,23 @@ TEST_F(Lookup, enoent) EXPECT_EQ(ENOENT, errno); } -//TODO: test ENOTDIR +TEST_F(Lookup, enotdir) +{ + const char FULLPATH[] = "mountpoint/not_a_dir/some_file.txt"; + const char RELPATH[] = "not_a_dir"; + EXPECT_LOOKUP(1, RELPATH) + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, entry); + out->body.entry.entry_valid = UINT64_MAX; + out->body.entry.attr.mode = S_IFREG | 0644; + out->body.entry.nodeid = 42; + }))); + + ASSERT_EQ(-1, access(FULLPATH, F_OK)); + ASSERT_EQ(ENOTDIR, errno); +} + /* * If lookup returns a non-zero entry timeout, then subsequent VOP_LOOKUPs * should use the cached inode rather than requery the daemon @@ -291,5 +306,3 @@ TEST_F(Lookup, subdir) */ ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno); } - - Modified: projects/fuse2/tests/sys/fs/fusefs/setattr.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/setattr.cc Wed Apr 10 20:44:54 2019 (r346100) +++ projects/fuse2/tests/sys/fs/fusefs/setattr.cc Wed Apr 10 21:10:21 2019 (r346101) @@ -41,7 +41,15 @@ using namespace testing; class Setattr : public FuseTest {}; +class RofsSetattr: public Setattr { +public: +virtual void SetUp() { + m_ro = true; + Setattr::SetUp(); +} +}; + /* * If setattr returns a non-zero cache timeout, then subsequent VOP_GETATTRs * should use the cached attributes, rather than query the daemon @@ -103,7 +111,6 @@ TEST_F(Setattr, chmod) SET_OUT_HEADER_LEN(out, entry); out->body.entry.attr.mode = S_IFREG | oldmode; out->body.entry.nodeid = ino; - out->body.entry.attr.mode = S_IFREG | oldmode; }))); EXPECT_CALL(*m_mock, process( @@ -554,4 +561,22 @@ TEST_F(Setattr, utimensat_mtime_only) { << strerror(errno); } -// TODO: test for erofs +/* On a read-only mount, no attributes may be changed */ +TEST_F(RofsSetattr, erofs) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + const uint64_t ino = 42; + const mode_t oldmode = 0755; + const mode_t newmode = 0644; + + EXPECT_LOOKUP(1, RELPATH) + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, entry); + out->body.entry.attr.mode = S_IFREG | oldmode; + out->body.entry.nodeid = ino; + }))); + + ASSERT_EQ(-1, chmod(FULLPATH, newmode)); + ASSERT_EQ(EROFS, errno); +} Modified: projects/fuse2/tests/sys/fs/fusefs/xattr.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/xattr.cc Wed Apr 10 20:44:54 2019 (r346100) +++ projects/fuse2/tests/sys/fs/fusefs/xattr.cc Wed Apr 10 21:10:21 2019 (r346101) @@ -108,6 +108,13 @@ class Getxattr: public Xattr {}; class Listxattr: public Xattr {}; class Removexattr: public Xattr {}; class Setxattr: public Xattr {}; +class RofsXattr: public Xattr { +public: +virtual void SetUp() { + m_ro = true; + Xattr::SetUp(); +} +}; /* * If the extended attribute does not exist on this file, the daemon should @@ -602,6 +609,32 @@ TEST_F(Setxattr, system) r = extattr_set_file(FULLPATH, ns, "foo", (void*)value, value_len); ASSERT_EQ(value_len, r) << strerror(errno); +} + +TEST_F(RofsXattr, deleteextattr_erofs) +{ + uint64_t ino = 42; + int ns = EXTATTR_NAMESPACE_USER; + + expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1); + + ASSERT_EQ(-1, extattr_delete_file(FULLPATH, ns, "foo")); + ASSERT_EQ(EROFS, errno); +} + +TEST_F(RofsXattr, setextattr_erofs) +{ + uint64_t ino = 42; + const char value[] = "whatever"; + ssize_t value_len = strlen(value) + 1; + int ns = EXTATTR_NAMESPACE_USER; + ssize_t r; + + expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1); + + r = extattr_set_file(FULLPATH, ns, "foo", (void*)value, value_len); + ASSERT_EQ(-1, r); + EXPECT_EQ(EROFS, errno); } // TODO: EROFS tests
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904102110.x3ALAMBo068004>