lkml.org 
[lkml]   [2009]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 06/13] sysfs: Fix locking and factor out sysfs_sd_setattr
    From
    Date
    ebiederm@xmission.com (Eric W. Biederman) writes:

    > "Serge E. Hallyn" <serue@us.ibm.com> writes:
    >
    >> Quoting Eric W. Biederman (ebiederm@xmission.com):
    >>> From: Eric W. Biederman <ebiederm@xmission.com>
    >>>
    >>> Cleanly separate the work that is specific to setting the
    >>> attributes of a sysfs_dirent from what is needed to update
    >>> the attributes of a vfs inode.
    >>>
    >>> Additionally grab the sysfs_mutex to keep any nasties from
    >>> surprising us when updating the sysfs_dirent.
    >>>
    >>> Acked-by: Tejun Heo <tj@kernel.org>
    >>> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
    >>> ---
    >>> fs/sysfs/inode.c | 52 ++++++++++++++++++++++++++++++++--------------------
    >>> fs/sysfs/sysfs.h | 1 +
    >>> 2 files changed, 33 insertions(+), 20 deletions(-)
    >>>
    >>> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
    >>> index fed7a74..fccfb55 100644
    >>> --- a/fs/sysfs/inode.c
    >>> +++ b/fs/sysfs/inode.c
    >>> @@ -64,30 +64,15 @@ struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
    >>>
    >>> return attrs;
    >>> }
    >>> -int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
    >>> +
    >>> +int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
    >>> {
    >>> - struct inode * inode = dentry->d_inode;
    >>> - struct sysfs_dirent * sd = dentry->d_fsdata;
    >>> struct sysfs_inode_attrs *sd_attrs;
    >>> struct iattr *iattrs;
    >>> unsigned int ia_valid = iattr->ia_valid;
    >>> - int error;
    >>> -
    >>> - if (!sd)
    >>> - return -EINVAL;
    >>>
    >>> sd_attrs = sd->s_iattr;
    >>>
    >>> - error = inode_change_ok(inode, iattr);
    >>> - if (error)
    >>> - return error;
    >>> -
    >>> - iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
    >>> -
    >>> - error = inode_setattr(inode, iattr);
    >>> - if (error)
    >>> - return error;
    >>> -
    >>> if (!sd_attrs) {
    >>> /* setting attributes for the first time, allocate now */
    >>> sd_attrs = sysfs_init_inode_attrs(sd);
    >>> @@ -110,12 +95,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
    >>> iattrs->ia_ctime = iattr->ia_ctime;
    >>> if (ia_valid & ATTR_MODE) {
    >>> umode_t mode = iattr->ia_mode;
    >>> -
    >>> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
    >>> - mode &= ~S_ISGID;
    >>> iattrs->ia_mode = sd->s_mode = mode;
    >>> }
    >>> }
    >>> + return 0;
    >>> +}
    >>> +
    >>> +int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
    >>> +{
    >>> + struct inode * inode = dentry->d_inode;
    >>> + struct sysfs_dirent * sd = dentry->d_fsdata;
    >>> + int error;
    >>> +
    >>> + if (!sd)
    >>> + return -EINVAL;
    >>> +
    >>> + error = inode_change_ok(inode, iattr);
    >>> + if (error)
    >>> + return error;
    >>> +
    >>> + iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
    >>> + if (iattr->ia_valid & ATTR_MODE) {
    >>> + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
    >>> + iattr->ia_mode &= ~S_ISGID;
    >>> + }
    >>
    >> Was it a bug that before this patch this wasn't cleared before the
    >> actual inode_setattr()?
    >
    > Not strictly as inode_setattr performs the exact same check.
    >
    >> Since the S_ISGID will be set for the *new* gid, that is,
    >> iattr->ia_gid, shouldn't the user be required to be
    >> in_group_p(iattr->i_gid)? Note you haven't done the
    >> inode_setattr() yet.
    >
    > Interesting point, yes I am potentially testing the wrong gid
    > there. The dangers of moving code around.
    >
    > Thank you for catching that. !#@#!#

    Turns out it wasn't as bad as it looks because inode_change_ok already
    does what I am trying to do properly.

    Although come to think of it, SGID on a sysfs file is completely bogus.
    We don't exec files and we don't allow users to create anything.

    I have already sent the follow up patch to remove those few lines of code.

    Eric


    \
     
     \ /
      Last update: 2009-11-04 13:59    [W:4.976 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site