[patch] smack: remove unnecessary xattr checks - Kernel

This is a discussion on [patch] smack: remove unnecessary xattr checks - Kernel ; Hi Casey, This is an untested patch, if it looks OK, can you please apply it to your tree (or ACK it)? Thanks, Miklos ---- From: Miklos Szeredi getxattr() calls security_inode_permission(MAY_READ) so smack_inode_getxattr() is unnecessary. setxattr() and removexattr() call security_inode_permission(MAY_WRITE) ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [patch] smack: remove unnecessary xattr checks

  1. [patch] smack: remove unnecessary xattr checks

    Hi Casey,

    This is an untested patch, if it looks OK, can you please apply it to
    your tree (or ACK it)?

    Thanks,
    Miklos

    ----
    From: Miklos Szeredi

    getxattr() calls security_inode_permission(MAY_READ) so
    smack_inode_getxattr() is unnecessary.

    setxattr() and removexattr() call security_inode_permission(MAY_WRITE)
    so the write permission checks in smack_inode_setxattr() and
    smack_inode_removexattr() are unnecessary.

    Signed-off-by: Miklos Szeredi
    ---
    security/smack/smack_lsm.c | 19 -------------------
    1 file changed, 19 deletions(-)

    Index: linux-2.6/security/smack/smack_lsm.c
    ================================================== =================
    --- linux-2.6.orig/security/smack/smack_lsm.c 2008-07-01 21:44:05.000000000 +0200
    +++ linux-2.6/security/smack/smack_lsm.c 2008-07-01 21:45:27.000000000 +0200
    @@ -588,9 +588,6 @@ static int smack_inode_setxattr(struct d
    } else
    rc = cap_inode_setxattr(dentry, name, value, size, flags);

    - if (rc == 0)
    - rc = smk_curacc(smk_of_inode(dentry->d_inode), MAY_WRITE);
    -
    return rc;
    }

    @@ -636,18 +633,6 @@ static void smack_inode_post_setxattr(st
    }

    /*
    - * smack_inode_getxattr - Smack check on getxattr
    - * @dentry: the object
    - * @name: unused
    - *
    - * Returns 0 if access is permitted, an error code otherwise
    - */
    -static int smack_inode_getxattr(struct dentry *dentry, const char *name)
    -{
    - return smk_curacc(smk_of_inode(dentry->d_inode), MAY_READ);
    -}
    -
    -/*
    * smack_inode_removexattr - Smack check on removexattr
    * @dentry: the object
    * @name: name of the attribute
    @@ -668,9 +653,6 @@ static int smack_inode_removexattr(struc
    } else
    rc = cap_inode_removexattr(dentry, name);

    - if (rc == 0)
    - rc = smk_curacc(smk_of_inode(dentry->d_inode), MAY_WRITE);
    -
    return rc;
    }

    @@ -2606,7 +2588,6 @@ struct security_operations smack_ops = {
    .inode_getattr = smack_inode_getattr,
    .inode_setxattr = smack_inode_setxattr,
    .inode_post_setxattr = smack_inode_post_setxattr,
    - .inode_getxattr = smack_inode_getxattr,
    .inode_removexattr = smack_inode_removexattr,
    .inode_need_killpriv = cap_inode_need_killpriv,
    .inode_killpriv = cap_inode_killpriv,
    --
    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] smack: remove unnecessary xattr checks

    Miklos Szeredi wrote:
    > Hi Casey,
    >
    > This is an untested patch, if it looks OK, can you please apply it to
    > your tree (or ACK it)?
    >
    >


    I will give it some review and a test or two
    then let you know. Things are hopping right now,
    so it may take a day or two.

    Thank you for the work you've put into this.

    > Thanks,
    > Miklos
    >
    > ----
    > From: Miklos Szeredi
    >
    > getxattr() calls security_inode_permission(MAY_READ) so
    > smack_inode_getxattr() is unnecessary.
    >
    > setxattr() and removexattr() call security_inode_permission(MAY_WRITE)
    > so the write permission checks in smack_inode_setxattr() and
    > smack_inode_removexattr() are unnecessary.
    >
    > Signed-off-by: Miklos Szeredi
    > ---
    > security/smack/smack_lsm.c | 19 -------------------
    > 1 file changed, 19 deletions(-)
    >
    > Index: linux-2.6/security/smack/smack_lsm.c
    > ================================================== =================
    > --- linux-2.6.orig/security/smack/smack_lsm.c 2008-07-01 21:44:05.000000000 +0200
    > +++ linux-2.6/security/smack/smack_lsm.c 2008-07-01 21:45:27.000000000 +0200
    > @@ -588,9 +588,6 @@ static int smack_inode_setxattr(struct d
    > } else
    > rc = cap_inode_setxattr(dentry, name, value, size, flags);
    >
    > - if (rc == 0)
    > - rc = smk_curacc(smk_of_inode(dentry->d_inode), MAY_WRITE);
    > -
    > return rc;
    > }
    >
    > @@ -636,18 +633,6 @@ static void smack_inode_post_setxattr(st
    > }
    >
    > /*
    > - * smack_inode_getxattr - Smack check on getxattr
    > - * @dentry: the object
    > - * @name: unused
    > - *
    > - * Returns 0 if access is permitted, an error code otherwise
    > - */
    > -static int smack_inode_getxattr(struct dentry *dentry, const char *name)
    > -{
    > - return smk_curacc(smk_of_inode(dentry->d_inode), MAY_READ);
    > -}
    > -
    > -/*
    > * smack_inode_removexattr - Smack check on removexattr
    > * @dentry: the object
    > * @name: name of the attribute
    > @@ -668,9 +653,6 @@ static int smack_inode_removexattr(struc
    > } else
    > rc = cap_inode_removexattr(dentry, name);
    >
    > - if (rc == 0)
    > - rc = smk_curacc(smk_of_inode(dentry->d_inode), MAY_WRITE);
    > -
    > return rc;
    > }
    >
    > @@ -2606,7 +2588,6 @@ struct security_operations smack_ops = {
    > .inode_getattr = smack_inode_getattr,
    > .inode_setxattr = smack_inode_setxattr,
    > .inode_post_setxattr = smack_inode_post_setxattr,
    > - .inode_getxattr = smack_inode_getxattr,
    > .inode_removexattr = smack_inode_removexattr,
    > .inode_need_killpriv = cap_inode_need_killpriv,
    > .inode_killpriv = cap_inode_killpriv,
    >
    >
    >



    --

    ----------------------

    Casey Schaufler
    casey@schaufler-ca.com
    650.906.1780


    --
    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] smack: remove unnecessary xattr checks

    Miklos Szeredi wrote:
    > Hi Casey,
    >
    > This is an untested patch, if it looks OK, can you please apply it to
    > your tree (or ACK it)?
    >
    > Thanks,
    > Miklos
    >
    > ----
    > From: Miklos Szeredi
    >
    > getxattr() calls security_inode_permission(MAY_READ) so
    > smack_inode_getxattr() is unnecessary.
    >
    > setxattr() and removexattr() call security_inode_permission(MAY_WRITE)
    > so the write permission checks in smack_inode_setxattr() and
    > smack_inode_removexattr() are unnecessary.
    >
    > Signed-off-by: Miklos Szeredi
    >

    Nacked-by: Casey Schaufler

    I tried your patch without looking at it and found that
    getxattr is too permissive with your changes. I found that

    % ls -l foo

    will fail while

    % attr -S -g SMACK64 foo

    will succeed. Of course if stat() fails due to a Smack
    access check getxattr() ought to as well. So it would
    appear that the call to security_inode_permission is not
    sufficient.


    > ---
    > security/smack/smack_lsm.c | 19 -------------------
    > 1 file changed, 19 deletions(-)
    >
    > Index: linux-2.6/security/smack/smack_lsm.c
    > ================================================== =================
    > --- linux-2.6.orig/security/smack/smack_lsm.c 2008-07-01 21:44:05.000000000 +0200
    > +++ linux-2.6/security/smack/smack_lsm.c 2008-07-01 21:45:27.000000000 +0200
    > @@ -588,9 +588,6 @@ static int smack_inode_setxattr(struct d
    > } else
    > rc = cap_inode_setxattr(dentry, name, value, size, flags);
    >
    > - if (rc == 0)
    > - rc = smk_curacc(smk_of_inode(dentry->d_inode), MAY_WRITE);
    > -
    > return rc;
    > }
    >
    > @@ -636,18 +633,6 @@ static void smack_inode_post_setxattr(st
    > }
    >
    > /*
    > - * smack_inode_getxattr - Smack check on getxattr
    > - * @dentry: the object
    > - * @name: unused
    > - *
    > - * Returns 0 if access is permitted, an error code otherwise
    > - */
    > -static int smack_inode_getxattr(struct dentry *dentry, const char *name)
    > -{
    > - return smk_curacc(smk_of_inode(dentry->d_inode), MAY_READ);
    > -}
    > -
    > -/*
    > * smack_inode_removexattr - Smack check on removexattr
    > * @dentry: the object
    > * @name: name of the attribute
    > @@ -668,9 +653,6 @@ static int smack_inode_removexattr(struc
    > } else
    > rc = cap_inode_removexattr(dentry, name);
    >
    > - if (rc == 0)
    > - rc = smk_curacc(smk_of_inode(dentry->d_inode), MAY_WRITE);
    > -
    > return rc;
    > }
    >
    > @@ -2606,7 +2588,6 @@ struct security_operations smack_ops = {
    > .inode_getattr = smack_inode_getattr,
    > .inode_setxattr = smack_inode_setxattr,
    > .inode_post_setxattr = smack_inode_post_setxattr,
    > - .inode_getxattr = smack_inode_getxattr,
    > .inode_removexattr = smack_inode_removexattr,
    > .inode_need_killpriv = cap_inode_need_killpriv,
    > .inode_killpriv = cap_inode_killpriv,
    >
    >
    >



    --

    ----------------------

    Casey Schaufler
    casey@schaufler-ca.com
    650.906.1780


    --
    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] smack: remove unnecessary xattr checks

    On Tue, 01 Jul 2008, Casey Schaufler wrote:
    > I tried your patch without looking at it and found that
    > getxattr is too permissive with your changes. I found that
    >
    > % ls -l foo
    >
    > will fail while
    >
    > % attr -S -g SMACK64 foo
    >
    > will succeed. Of course if stat() fails due to a Smack
    > access check getxattr() ought to as well. So it would
    > appear that the call to security_inode_permission is not
    > sufficient.


    Hmm, I missed the fact that security_inode_permission() is only called
    for xattrs not in the speclial (security.*, system.*, trusted.*)
    namespaces. So yes the patch is incorrect.

    Thanks,
    Miklos
    --
    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