On Thu, 2008-02-28 at 15:10 -0500, Josef 'Jeff' Sipek wrote:
> Ignoring the security parts of it, here are a few comments about the VFS and
> coding style related bits...
>
> Josef 'Jeff' Sipek.
>
> On Wed, Feb 27, 2008 at 03:39:38PM -0500, David P. Quigley wrote:
> > This patch adds two new fields to the iattr structure. The first field holds a
> > security label while the second contains the length of this label. In addition
> > the patch adds a new helper function inode_setsecurity which calls the LSM to
> > set the security label on the inode. Finally the patch modifies the necessary
> > functions such that fsnotify_change can handle notification requests for
> > dnotify and inotify.
> >
> > Signed-off-by: David P. Quigley <dpquigl@xxxxxxxxxxxxx>
> > ---
> > fs/attr.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> > fs/xattr.c | 33 +++++++++++++++++++++++++++------
> > include/linux/fcntl.h | 1 +
> > include/linux/fs.h | 11 +++++++++++
> > include/linux/fsnotify.h | 6 ++++++
> > include/linux/inotify.h | 3 ++-
> > include/linux/xattr.h | 1 +
> > 7 files changed, 91 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index 966b73e..1df6603 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -5,6 +5,7 @@
> > * changes by Thomas Schoebel-Theuer
> > */
> >
> > +#include <linux/fs.h>
> > #include <linux/module.h>
> > #include <linux/time.h>
> > #include <linux/mm.h>
> > @@ -14,9 +15,35 @@
> > #include <linux/fcntl.h>
> > #include <linux/quotaops.h>
> > #include <linux/security.h>
> > +#include <linux/xattr.h>
> >
> > /* Taken over from the old code... */
> >
> > +#ifdef CONFIG_SECURITY
> > +/*
> > + * Update the in core label.
> > + */
> > +int inode_setsecurity(struct inode *inode, struct iattr *attr)
> > +{
> > + const char *suffix = security_maclabel_getname()
> > + + XATTR_SECURITY_PREFIX_LEN;
> > + int error;
> > +
> > + if (!attr->ia_valid & ATTR_SECURITY_LABEL)
> > + return -EINVAL;
>
> You most likely want:
>
> if (!(attr->ia_valid & ATTR_SECURITY_LABEL))
>
> IOW, watch out for operator precedence.
Already raised by James and fixed but thanks for catching it again.
>
> > +
> > + error = security_inode_setsecurity(inode, suffix, attr->ia_label,
> > + attr->ia_label_len, 0);
> > + if (error)
> > + printk(KERN_ERR "%s() %s %d security_inode_setsecurity() %d\n"
> > + , __func__, (char *)attr->ia_label, attr->ia_label_len
> > + , error);
>
> The commas should really be on the previous line.
Fixed.
>
> > +
> > + return error;
> > +}
> > +EXPORT_SYMBOL(inode_setsecurity);
> > +#endif
> > +
> > /* POSIX UID/GID verification for setting inode attributes. */
> > int inode_change_ok(struct inode *inode, struct iattr *attr)
> > {
> > @@ -94,6 +121,10 @@ int inode_setattr(struct inode * inode, struct iattr * attr)
> > mode &= ~S_ISGID;
> > inode->i_mode = mode;
> > }
> > +#ifdef CONFIG_SECURITY
> > + if (ia_valid & ATTR_SECURITY_LABEL)
> > + inode_setsecurity(inode, attr);
> > +#endif
> > mark_inode_dirty(inode);
> >
> > return 0;
> > @@ -157,6 +188,18 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
> > if (ia_valid & ATTR_SIZE)
> > down_write(&dentry->d_inode->i_alloc_sem);
> >
> > +#ifdef CONFIG_SECURITY
> > + if (ia_valid & ATTR_SECURITY_LABEL) {
> > + char *key = (char *)security_maclabel_getname();
> > + vfs_setxattr_locked(dentry, key,
> > + attr->ia_label, attr->ia_label_len, 0);
> > + /* Avoid calling inode_setsecurity()
> > + * via inode_setattr() below
> > + */
> > + attr->ia_valid &= ~ATTR_SECURITY_LABEL;
> > + }
> > +#endif
> > +
> > if (inode->i_op && inode->i_op->setattr) {
> > error = security_inode_setattr(dentry, attr);
> > if (!error)
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 3acab16..b5a91e1 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -67,9 +67,9 @@ xattr_permission(struct inode *inode, const char *name, int mask)
> > return permission(inode, mask, NULL);
> > }
> >
> > -int
> > -vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > - size_t size, int flags)
> > +static int
> > +_vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > + size_t size, int flags, int lock)
>
> The convention is to use __ or do_ as the prefix.
Fixed (added another _ to the beginning.)
>
> > {
> > struct inode *inode = dentry->d_inode;
> > int error;
> > @@ -78,7 +78,8 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > if (error)
> > return error;
> >
> > - mutex_lock(&inode->i_mutex);
> > + if (lock)
> > + mutex_lock(&inode->i_mutex);
> > error = security_inode_setxattr(dentry, name, value, size, flags);
> > if (error)
> > goto out;
> > @@ -95,15 +96,35 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> > error = security_inode_setsecurity(inode, suffix, value,
> > size, flags);
> > - if (!error)
> > + if (!error) {
> > +#ifdef CONFIG_SECURITY
> > + fsnotify_change(dentry, ATTR_SECURITY_LABEL);
> > +#endif
> > fsnotify_xattr(dentry);
> > + }
> > }
> > out:
> > - mutex_unlock(&inode->i_mutex);
> > + if (lock)
> > + mutex_unlock(&inode->i_mutex);
> > return error;
> > }
> > +
> > +int
> > +vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > + size_t size, int flags)
> > +{
> > + return _vfs_setxattr(dentry, name, value, size, flags, 1);
> > +}
> > EXPORT_SYMBOL_GPL(vfs_setxattr);
> >
> > +int
> > +vfs_setxattr_locked(struct dentry *dentry, char *name, void *value,
> > + size_t size, int flags)
> > +{
> > + return _vfs_setxattr(dentry, name, value, size, flags, 0);
> > +}
> > +EXPORT_SYMBOL_GPL(vfs_setxattr_locked);
>
> Alright...so, few things...
>
> 1) why do you need the locked/unlocked versions?
>
> 2) instead of passing a flag to a common function, why not have:
>
> vfs_setxattr_locked(....)
> {
> // original code minus the lock/unlock calls
> }
>
> vfs_setxattr(....)
> {
> mutex_lock(...);
> vfs_setxattr_locked(...);
> mutex_unlock(...);
> }
What we do and what you propose aren't logically equivalent. There is a
permission check inside vfs_setxattr before the mutex lock. However
looking at through the xattr_permission function and its call chain it
doesn't seem like we would create a deadlock by locking the inode before
it is called; so it is possible to do what you propose. Since setting of
xattrs (at least from our perspective) is a less common operation I
don't think putting locking around the entire call would make that large
of a difference.
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
