Re: vaccess() and ntfs_access() and...

[Elad Efrat <elad%NetBSD.org@localhost>]

David Holland wrote:
> On Wed, Apr 22, 2009 at 02:30:35AM +0300, Elad Efrat wrote:
>  > It looks like vaccess() in and ntfs_access() are mostly the same,
>  > except for:
>  > 
>  >   1. ntfs_access() disallows writing to a read-only file-system unless
>  >      writing to a socket, fifo, or a block device
>  > 
>  >   2. vaccess() requires at least one exec bit to allow execution (for
>  >      the superuser)
>  > 
>  > Are we interested in uniting them, ie., make vaccess() enforce #1 and
>  > ntfs care about #2? (so we can just call vaccess() from ntfs_access()
>  > like all other file-systems)
>
> I don't know. But:

Okay. The attached diff preserves behavior and allows me to remove the
KAUTH_GENERIC_ISSUSER call from there (we now call vaccess()). I'll go
ahead and commit it if there are no objections on the list.

(btw, I'm not too familiar with ntfs/msdosfs, but shouldn't there be a
read-only check in ntfs as well? also, I think the read-only bit is
enforced regardless of the user's privileges.)

>  > Speaking of all other file-systems, is there a reason some don't care
>  > if the file-system is read-only and some do? and among those who do,
>  > some care about the vnode type (VREG, VDIR, ...) and some don't?
>
> Accumulation of entropy. If you feel like performing a net input of
> energy, it would be a good thing, but the first step has to be to wade
> through all of them (and the call sites, etc.) to figure out what the
> right set of checks is.

I will wade through them when I get the time. It's not a priority, and
at the moment I just want to eliminate the KAUTH_GENERIC_ISSUSER call
in ntfs_access().

That said, I think adding the "if the mount is read-only, disallow write
if VDIR/VREG/VLNK" to vaccess() makes sense. After a quick look, I'm
almost sure everything else is file-system specific and should stay that
way.

>  > and that some use the file-system node (FSNAME_VTOI(vp)) to get the
>  > uid/gid etc., and some VOP_GETATTR()?
>
> Given layers I believe the former are wrong, but I'd want a second or
> third opinion before taking any steps.

Not suggesting anything, but why is it wrong, layers-wise, for
file-system code to access members of file-system inodes?

Thanks,

-e.

Index: ntfs_vnops.c
===================================================================
RCS file: /usr/cvs/src/sys/fs/ntfs/ntfs_vnops.c,v
retrieving revision 1.42
diff -u -p -r1.42 ntfs_vnops.c
--- ntfs_vnops.c        17 Dec 2008 20:51:35 -0000      1.42
+++ ntfs_vnops.c        26 Apr 2009 06:15:00 -0000
@@ -405,11 +405,7 @@ ntfs_access(void *v)
        } */ *ap = v;
        struct vnode *vp = ap->a_vp;
        struct ntnode *ip = VTONT(vp);
-       kauth_cred_t cred = ap->a_cred;
-       mode_t mask, mode = ap->a_mode;
-       gid_t grp;
-       int i;
-       uint16_t ngroups;
+       mode_t file_mode, mode = ap->a_mode;
 
        dprintf(("ntfs_access: %llu\n", (unsigned long long)ip->i_number));
 
@@ -429,46 +425,10 @@ ntfs_access(void *v)
                }
        }
 
-       /* Otherwise, user id 0 always gets access. */
-       if (kauth_authorize_generic(cred, KAUTH_GENERIC_ISSUSER, NULL) == 0)
-               return (0);
-
-       mask = 0;
-
-       /* Otherwise, check the owner. */
-       if (kauth_cred_geteuid(cred) == ip->i_mp->ntm_uid) {
-               if (mode & VEXEC)
-                       mask |= S_IXUSR;
-               if (mode & VREAD)
-                       mask |= S_IRUSR;
-               if (mode & VWRITE)
-                       mask |= S_IWUSR;
-               return ((ip->i_mp->ntm_mode & mask) == mask ? 0 : EACCES);
-       }
-
-       /* Otherwise, check the groups. */
-       ngroups = kauth_cred_ngroups(cred);
-       for (i = 0; i < ngroups; i++) {
-               grp = kauth_cred_group(cred, i);
-               if (ip->i_mp->ntm_gid == grp) {
-                       if (mode & VEXEC)
-                               mask |= S_IXGRP;
-                       if (mode & VREAD)
-                               mask |= S_IRGRP;
-                       if (mode & VWRITE)
-                               mask |= S_IWGRP;
-                       return ((ip->i_mp->ntm_mode&mask) == mask ? 0 : EACCES);
-               }
-       }
+       file_mode = ip->i_mp->ntm_mode | (S_IXUSR|S_IXGRP|S_IXOTH);
 
-       /* Otherwise, check everyone else. */
-       if (mode & VEXEC)
-               mask |= S_IXOTH;
-       if (mode & VREAD)
-               mask |= S_IROTH;
-       if (mode & VWRITE)
-               mask |= S_IWOTH;
-       return ((ip->i_mp->ntm_mode & mask) == mask ? 0 : EACCES);
+       return (vaccess(vp->v_type, file_mode, ip->i_mp->ntm_uid,
+           ip->i_mp->ntm_gid, mode, ap->a_cred));
 }
 
 /*