[patch resend] vfs: move executable checking into ->permission() - Kernel

This is a discussion on [patch resend] vfs: move executable checking into ->permission() - Kernel ; A ->permission() API cleanup patch that I missed resending... Please apply to 2.6.27 or .28 at your discretion. Thanks, Miklos ---- From: Miklos Szeredi For execute permission on a regular files we need to check if file has any execute ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [patch resend] vfs: move executable checking into ->permission()

  1. [patch resend] vfs: move executable checking into ->permission()

    A ->permission() API cleanup patch that I missed resending...

    Please apply to 2.6.27 or .28 at your discretion.

    Thanks,
    Miklos

    ----
    From: Miklos Szeredi

    For execute permission on a regular files we need to check if file has
    any execute bits at all, regardless of capabilites.

    This check is normally performed by generic_permission() but was also
    added to the case when the filesystem defines its own ->permission()
    method. In the latter case the filesystem should be responsible for
    performing this check.

    So create a helper function check_execute() that returns -EACCESS if
    MAY_EXEC is present, the inode is a regular file and no execute bits
    are present in inode->i_mode.

    Call this function from filesystems which don't call
    generic_permission() from their ->permission() methods and which
    aren't already performing this check. Also remove the check from
    dentry_permission().

    The new code should be equivalent to the old.

    [Tetsuo Handa : export
    check_execute() to modules]

    Signed-off-by: Miklos Szeredi
    ---
    fs/cifs/cifsfs.c | 2 +-
    fs/coda/dir.c | 4 ++++
    fs/coda/pioctl.c | 2 +-
    fs/hfs/inode.c | 2 +-
    fs/namei.c | 39 ++++++++++++++++++++++++---------------
    fs/nfs/dir.c | 3 +++
    fs/proc/proc_sysctl.c | 3 +++
    fs/smbfs/file.c | 6 +++---
    include/linux/fs.h | 1 +
    9 files changed, 41 insertions(+), 21 deletions(-)

    Index: linux-2.6/fs/cifs/cifsfs.c
    ================================================== =================
    --- linux-2.6.orig/fs/cifs/cifsfs.c 2008-07-30 14:38:22.000000000 +0200
    +++ linux-2.6/fs/cifs/cifsfs.c 2008-07-30 14:39:31.000000000 +0200
    @@ -274,7 +274,7 @@ static int cifs_permission(struct inode
    cifs_sb = CIFS_SB(inode->i_sb);

    if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM)
    - return 0;
    + return check_execute(inode, mask);
    else /* file mode might have been restricted at mount time
    on the client (above and beyond ACL on servers) for
    servers which do not support setting and viewing mode bits,
    Index: linux-2.6/fs/coda/dir.c
    ================================================== =================
    --- linux-2.6.orig/fs/coda/dir.c 2008-07-30 14:38:22.000000000 +0200
    +++ linux-2.6/fs/coda/dir.c 2008-07-30 14:39:31.000000000 +0200
    @@ -146,6 +146,10 @@ int coda_permission(struct inode *inode,
    if (!mask)
    return 0;

    + error = check_execute(inode, mask);
    + if (error)
    + return error;
    +
    lock_kernel();

    if (coda_cache_check(inode, mask))
    Index: linux-2.6/fs/coda/pioctl.c
    ================================================== =================
    --- linux-2.6.orig/fs/coda/pioctl.c 2008-07-30 14:38:22.000000000 +0200
    +++ linux-2.6/fs/coda/pioctl.c 2008-07-30 14:39:31.000000000 +0200
    @@ -43,7 +43,7 @@ const struct file_operations coda_ioctl_
    /* the coda pioctl inode ops */
    static int coda_ioctl_permission(struct inode *inode, int mask)
    {
    - return 0;
    + return check_execute(dentry->d_inode, mask);
    }

    static int coda_pioctl(struct inode * inode, struct file * filp,
    Index: linux-2.6/fs/hfs/inode.c
    ================================================== =================
    --- linux-2.6.orig/fs/hfs/inode.c 2008-07-30 14:38:22.000000000 +0200
    +++ linux-2.6/fs/hfs/inode.c 2008-07-30 14:39:31.000000000 +0200
    @@ -514,7 +514,7 @@ void hfs_clear_inode(struct inode *inode
    static int hfs_permission(struct inode *inode, int mask)
    {
    if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
    - return 0;
    + return check_execute(inode, mask);
    return generic_permission(inode, mask, NULL);
    }

    Index: linux-2.6/fs/namei.c
    ================================================== =================
    --- linux-2.6.orig/fs/namei.c 2008-07-30 14:39:31.000000000 +0200
    +++ linux-2.6/fs/namei.c 2008-07-30 14:39:31.000000000 +0200
    @@ -227,6 +227,27 @@ int generic_permission(struct inode *ino
    return -EACCES;
    }

    +/**
    + * check_execute - check for general execute permission on file
    + * @inode: inode to check access rights for
    + * @mask: right to check for
    + *
    + * Exec permission on a regular file is denied if none of the execute
    + * bits are set.
    + *
    + * This needs to be called by filesystems which define a
    + * ->permission() method, and don't call generic_permission().
    + */
    +int check_execute(struct inode *inode, int mask)
    +{
    + if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
    + !(inode->i_mode & S_IXUGO))
    + return -EACCES;
    +
    + return 0;
    +}
    +EXPORT_SYMBOL(check_execute);
    +
    int inode_permission(struct inode *inode, int mask)
    {
    int retval;
    @@ -249,23 +270,11 @@ int inode_permission(struct inode *inode
    }

    /* Ordinary permission routines do not understand MAY_APPEND. */
    - if (inode->i_op && inode->i_op->permission) {
    + if (inode->i_op && inode->i_op->permission)
    retval = inode->i_op->permission(inode, mask);
    - if (!retval) {
    - /*
    - * Exec permission on a regular file is denied if none
    - * of the execute bits are set.
    - *
    - * This check should be done by the ->permission()
    - * method.
    - */
    - if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
    - !(inode->i_mode & S_IXUGO))
    - return -EACCES;
    - }
    - } else {
    + else
    retval = generic_permission(inode, mask, NULL);
    - }
    +
    if (retval)
    return retval;

    Index: linux-2.6/fs/nfs/dir.c
    ================================================== =================
    --- linux-2.6.orig/fs/nfs/dir.c 2008-07-30 14:38:22.000000000 +0200
    +++ linux-2.6/fs/nfs/dir.c 2008-07-30 14:39:31.000000000 +0200
    @@ -1949,6 +1949,9 @@ force_lookup:
    } else
    res = PTR_ERR(cred);
    out:
    + if (res == 0)
    + res = check_execute(inode, mask);
    +
    dfprintk(VFS, "NFS: permission(%s/%ld), mask=0x%x, res=%d\n",
    inode->i_sb->s_id, inode->i_ino, mask, res);
    return res;
    Index: linux-2.6/fs/proc/proc_sysctl.c
    ================================================== =================
    --- linux-2.6.orig/fs/proc/proc_sysctl.c 2008-07-30 14:38:22.000000000 +0200
    +++ linux-2.6/fs/proc/proc_sysctl.c 2008-07-30 14:39:31.000000000 +0200
    @@ -311,6 +311,9 @@ static int proc_sys_permission(struct in
    error = sysctl_perm(head->root, table, mask);

    sysctl_head_finish(head);
    + if (!error)
    + error = check_execute(inode, mask);
    +
    return error;
    }

    Index: linux-2.6/include/linux/fs.h
    ================================================== =================
    --- linux-2.6.orig/include/linux/fs.h 2008-07-30 14:39:31.000000000 +0200
    +++ linux-2.6/include/linux/fs.h 2008-07-30 14:39:31.000000000 +0200
    @@ -1777,6 +1777,7 @@ extern int notify_change(struct dentry *
    extern int inode_permission(struct inode *, int);
    extern int generic_permission(struct inode *, int,
    int (*check_acl)(struct inode *, int));
    +extern int check_execute(struct inode *, int);

    extern int get_write_access(struct inode *);
    extern int deny_write_access(struct file *);
    Index: linux-2.6/fs/smbfs/file.c
    ================================================== =================
    --- linux-2.6.orig/fs/smbfs/file.c 2008-07-30 14:38:22.000000000 +0200
    +++ linux-2.6/fs/smbfs/file.c 2008-07-30 14:39:31.000000000 +0200
    @@ -411,15 +411,15 @@ static int
    smb_file_permission(struct inode *inode, int mask)
    {
    int mode = inode->i_mode;
    - int error = 0;

    VERBOSE("mode=%x, mask=%x\n", mode, mask);

    /* Look at user permissions */
    mode >>= 6;
    if (mask & ~mode & (MAY_READ | MAY_WRITE | MAY_EXEC))
    - error = -EACCES;
    - return error;
    + return -EACCES;
    +
    + return check_execute(inode, mask);
    }

    static loff_t smb_remote_llseek(struct file *file, loff_t offset, int origin)
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [patch resend] vfs: move executable checking into ->permission()

    On Wed, Jul 30, 2008 at 03:02:03PM +0200, Miklos Szeredi wrote:
    > static int coda_ioctl_permission(struct inode *inode, int mask)
    > {
    > - return 0;
    > + return check_execute(dentry->d_inode, mask);
    > }


    Er?
    a) mismerge from dentry-based variant
    b) I'd say return mask & MAY_EXEC ? -EACCES : 0 - it's *NOT* going to
    be an executable file, TYVM.

    > static int hfs_permission(struct inode *inode, int mask)
    > {
    > if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
    > - return 0;
    > + return check_execute(inode, mask);
    > return generic_permission(inode, mask, NULL);
    > }


    WTF is going on in that one? I realize that you are not changing behaviour,
    but...

    > +int check_execute(struct inode *inode, int mask)
    > +{
    > + if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
    > + !(inode->i_mode & S_IXUGO))
    > + return -EACCES;
    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL(check_execute);


    Umm... I'm not sure. For one thing, I'd take check for MAY_EXEC to callers.
    For another, quite a few of those might have enough information to make calling
    that helper pointless.

    > +++ linux-2.6/fs/proc/proc_sysctl.c 2008-07-30 14:39:31.000000000 +0200
    > @@ -311,6 +311,9 @@ static int proc_sys_permission(struct in
    > error = sysctl_perm(head->root, table, mask);
    >
    > sysctl_head_finish(head);
    > + if (!error)
    > + error = check_execute(inode, mask);
    > +
    > return error;
    > }


    No. If anything, we want non-directories fail MAY_EXEC here, no matter
    what i_mode we might have. Executable files in /proc/sys/* are NOT going
    to be allowed, no matter what...

    > if (mask & ~mode & (MAY_READ | MAY_WRITE | MAY_EXEC))
    > - error = -EACCES;
    > - return error;
    > + return -EACCES;
    > +
    > + return check_execute(inode, mask);


    That's wrong. If mask contains MAY_EXEC and we got to calling check_execute(),
    we know that ~mode & MAY_EXEC is 0. IOW, we know that inode->i_mode >> 6 has
    bit 0 set. IOW, we know that inode->i_mode contain S_IXUSR. IOW, your
    check_execute() here is an obfuscated way to spell 0.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [patch resend] vfs: move executable checking into ->permission()

    On Thu, 31 Jul 2008, Al Viro wrote:
    > On Wed, Jul 30, 2008 at 03:02:03PM +0200, Miklos Szeredi wrote:
    > > static int coda_ioctl_permission(struct inode *inode, int mask)
    > > {
    > > - return 0;
    > > + return check_execute(dentry->d_inode, mask);
    > > }

    >
    > Er?
    > a) mismerge from dentry-based variant


    Yeah, crappy patch. Agree on all other comments as well, thanks for
    the review.

    Here's an updated one.

    Miklos
    ----

    Subject: vfs: move executable checking into ->permission()

    From: Miklos Szeredi

    For execute permission on a regular files we need to check if file has
    any execute bits at all, regardless of capabilites.

    This check is normally performed by generic_permission() but was also
    added to the case when the filesystem defines its own ->permission()
    method. In the latter case the filesystem should be responsible for
    performing this check.

    Move the check from inode_permission() inside filesystems which are
    not calling generic_permission().

    Create a helper function execute_ok() that returns true if the inode
    is a directory or if any execute bits are present in i_mode.

    Also fix up the following code:

    - coda control file is never executable
    - sysctl files are never executable
    - hfs_permission seems broken on MAY_EXEC, remove
    - hfsplus_permission is eqivalent to generic_permission(), remove

    Signed-off-by: Miklos Szeredi
    ---
    fs/cifs/cifsfs.c | 9 ++++++---
    fs/coda/dir.c | 3 +++
    fs/coda/pioctl.c | 2 +-
    fs/hfs/inode.c | 8 --------
    fs/hfsplus/inode.c | 13 -------------
    fs/namei.c | 21 ++++-----------------
    fs/nfs/dir.c | 3 +++
    fs/proc/proc_sysctl.c | 10 ++++++++--
    include/linux/fs.h | 5 +++++
    9 files changed, 30 insertions(+), 44 deletions(-)

    Index: linux-2.6/fs/cifs/cifsfs.c
    ================================================== =================
    --- linux-2.6.orig/fs/cifs/cifsfs.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/cifs/cifsfs.c 2008-07-31 13:28:11.000000000 +0200
    @@ -273,9 +273,12 @@ static int cifs_permission(struct inode

    cifs_sb = CIFS_SB(inode->i_sb);

    - if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM)
    - return 0;
    - else /* file mode might have been restricted at mount time
    + if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM) {
    + if ((mask & MAY_EXEC) && !execute_ok(inode))
    + return -EACCES;
    + else
    + return 0;
    + } else /* file mode might have been restricted at mount time
    on the client (above and beyond ACL on servers) for
    servers which do not support setting and viewing mode bits,
    so allowing client to check permissions is useful */
    Index: linux-2.6/fs/coda/dir.c
    ================================================== =================
    --- linux-2.6.orig/fs/coda/dir.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/coda/dir.c 2008-07-31 13:28:11.000000000 +0200
    @@ -146,6 +146,9 @@ int coda_permission(struct inode *inode,
    if (!mask)
    return 0;

    + if ((mask & MAY_EXEC) && !execute_ok(inode))
    + return -EACCES;
    +
    lock_kernel();

    if (coda_cache_check(inode, mask))
    Index: linux-2.6/fs/coda/pioctl.c
    ================================================== =================
    --- linux-2.6.orig/fs/coda/pioctl.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/coda/pioctl.c 2008-07-31 13:28:11.000000000 +0200
    @@ -43,7 +43,7 @@ const struct file_operations coda_ioctl_
    /* the coda pioctl inode ops */
    static int coda_ioctl_permission(struct inode *inode, int mask)
    {
    - return 0;
    + return (mask & MAY_EXEC) ? -EACCES : 0;
    }

    static int coda_pioctl(struct inode * inode, struct file * filp,
    Index: linux-2.6/fs/namei.c
    ================================================== =================
    --- linux-2.6.orig/fs/namei.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/namei.c 2008-07-31 13:28:11.000000000 +0200
    @@ -212,8 +212,7 @@ int generic_permission(struct inode *ino
    * Read/write DACs are always overridable.
    * Executable DACs are overridable if at least one exec bit is set.
    */
    - if (!(mask & MAY_EXEC) ||
    - (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode))
    + if (!(mask & MAY_EXEC) || execute_ok(inode))
    if (capable(CAP_DAC_OVERRIDE))
    return 0;

    @@ -249,23 +248,11 @@ int inode_permission(struct inode *inode
    }

    /* Ordinary permission routines do not understand MAY_APPEND. */
    - if (inode->i_op && inode->i_op->permission) {
    + if (inode->i_op && inode->i_op->permission)
    retval = inode->i_op->permission(inode, mask);
    - if (!retval) {
    - /*
    - * Exec permission on a regular file is denied if none
    - * of the execute bits are set.
    - *
    - * This check should be done by the ->permission()
    - * method.
    - */
    - if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
    - !(inode->i_mode & S_IXUGO))
    - return -EACCES;
    - }
    - } else {
    + else
    retval = generic_permission(inode, mask, NULL);
    - }
    +
    if (retval)
    return retval;

    Index: linux-2.6/fs/nfs/dir.c
    ================================================== =================
    --- linux-2.6.orig/fs/nfs/dir.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/nfs/dir.c 2008-07-31 13:28:11.000000000 +0200
    @@ -1949,6 +1949,9 @@ force_lookup:
    } else
    res = PTR_ERR(cred);
    out:
    + if (!res && (mask & MAY_EXEC) && !execute_ok(inode))
    + res = -EACCES;
    +
    dfprintk(VFS, "NFS: permission(%s/%ld), mask=0x%x, res=%d\n",
    inode->i_sb->s_id, inode->i_ino, mask, res);
    return res;
    Index: linux-2.6/include/linux/fs.h
    ================================================== =================
    --- linux-2.6.orig/include/linux/fs.h 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/include/linux/fs.h 2008-07-31 13:28:11.000000000 +0200
    @@ -1776,6 +1776,11 @@ extern int inode_permission(struct inode
    extern int generic_permission(struct inode *, int,
    int (*check_acl)(struct inode *, int));

    +static inline bool execute_ok(struct inode *inode)
    +{
    + return (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode);
    +}
    +
    extern int get_write_access(struct inode *);
    extern int deny_write_access(struct file *);
    static inline void put_write_access(struct inode * inode)
    Index: linux-2.6/fs/hfs/inode.c
    ================================================== =================
    --- linux-2.6.orig/fs/hfs/inode.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/hfs/inode.c 2008-07-31 13:28:21.000000000 +0200
    @@ -511,13 +511,6 @@ void hfs_clear_inode(struct inode *inode
    }
    }

    -static int hfs_permission(struct inode *inode, int mask)
    -{
    - if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
    - return 0;
    - return generic_permission(inode, mask, NULL);
    -}
    -
    static int hfs_file_open(struct inode *inode, struct file *file)
    {
    if (HFS_IS_RSRC(inode))
    @@ -616,7 +609,6 @@ static const struct inode_operations hfs
    .lookup = hfs_file_lookup,
    .truncate = hfs_file_truncate,
    .setattr = hfs_inode_setattr,
    - .permission = hfs_permission,
    .setxattr = hfs_setxattr,
    .getxattr = hfs_getxattr,
    .listxattr = hfs_listxattr,
    Index: linux-2.6/fs/hfsplus/inode.c
    ================================================== =================
    --- linux-2.6.orig/fs/hfsplus/inode.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/hfsplus/inode.c 2008-07-31 13:28:25.000000000 +0200
    @@ -238,18 +238,6 @@ static void hfsplus_set_perms(struct ino
    perms->dev = cpu_to_be32(HFSPLUS_I(inode).dev);
    }

    -static int hfsplus_permission(struct inode *inode, int mask)
    -{
    - /* MAY_EXEC is also used for lookup, if no x bit is set allow lookup,
    - * open_exec has the same test, so it's still not executable, if a x bit
    - * is set fall back to standard permission check.
    - */
    - if (S_ISREG(inode->i_mode) && mask & MAY_EXEC && !(inode->i_mode & 0111))
    - return 0;
    - return generic_permission(inode, mask, NULL);
    -}
    -
    -
    static int hfsplus_file_open(struct inode *inode, struct file *file)
    {
    if (HFSPLUS_IS_RSRC(inode))
    @@ -279,7 +267,6 @@ static int hfsplus_file_release(struct i
    static const struct inode_operations hfsplus_file_inode_operations = {
    .lookup = hfsplus_file_lookup,
    .truncate = hfsplus_file_truncate,
    - .permission = hfsplus_permission,
    .setxattr = hfsplus_setxattr,
    .getxattr = hfsplus_getxattr,
    .listxattr = hfsplus_listxattr,
    Index: linux-2.6/fs/proc/proc_sysctl.c
    ================================================== =================
    --- linux-2.6.orig/fs/proc/proc_sysctl.c 2008-07-31 13:27:44.000000000 +0200
    +++ linux-2.6/fs/proc/proc_sysctl.c 2008-07-31 13:28:32.000000000 +0200
    @@ -298,13 +298,19 @@ static int proc_sys_permission(struct in
    * sysctl entries that are not writeable,
    * are _NOT_ writeable, capabilities or not.
    */
    - struct ctl_table_header *head = grab_header(inode);
    - struct ctl_table *table = PROC_I(inode)->sysctl_entry;
    + struct ctl_table_header *head;
    + struct ctl_table *table;
    int error;

    + /* Executable files are not allowed under /proc/sys/ */
    + if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))
    + return -EACCES;
    +
    + head = grab_header(inode);
    if (IS_ERR(head))
    return PTR_ERR(head);

    + table = PROC_I(inode)->sysctl_entry;
    if (!table) /* global root - r-xr-xr-x */
    error = mask & MAY_WRITE ? -EACCES : 0;
    else /* Use the permissions on the sysctl table entry */
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [patch resend] vfs: move executable checking into ->permission()

    On Thu, Jul 31, 2008 at 01:41:58PM +0200, Miklos Szeredi wrote:
    > On Thu, 31 Jul 2008, Al Viro wrote:
    > > On Wed, Jul 30, 2008 at 03:02:03PM +0200, Miklos Szeredi wrote:
    > > > static int coda_ioctl_permission(struct inode *inode, int mask)
    > > > {
    > > > - return 0;
    > > > + return check_execute(dentry->d_inode, mask);
    > > > }

    > >
    > > Er?
    > > a) mismerge from dentry-based variant

    >
    > Yeah, crappy patch. Agree on all other comments as well, thanks for
    > the review.


    Hrm... OK, behaviour change for permission() on bdev/cdev/pipe is
    not a problems...

    I'll apply it, but if anybody has better suggestions for helper name,
    please yell. "execute_ok()" sounds very odd...
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread