| Message ID | 1440016553-26481-1-git-send-email-richard@nod.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/20/2015 04:35 AM, Richard Weinberger wrote: > This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53 > ("UBIFS: Add security.* XATTR support for the UBIFS"). Hi Richard, What about a full reverting of this commit. In ubifs, we *can* support any namespace of xattr including user, trusted, security or other anyone prefixed by any words. But we have a check_namespace() in xattr.c to limit what we want to support. That said, if we want to "Add security.* XATTR support for the UBIFS", what we need to do is just extending the check_namespace() to allow security namespace pass. And yes, check_namespace() have been supporting security namespace. So, IMHO, we do not depend on the generic mechanism at all, and we can fully revert this commit. But strange to me, why we picked this commit for ubifs? Artem, is there something I am missing? Yang > > As UBIFS does not use generic inode xattr inode operations > the code behind sb->s_xattr is never called. > Remove that dead code for now. > > Cc: Subodh Nijsure <snijsure@grid-net.com> > Cc: Marc Kleine-Budde <mkl@pengutronix.de> > Cc: Brad Mouring <brad.mouring@ni.com> > Cc: Gratian Crisan <gratian.crisan@ni.com> > Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> > Reported-by: Andreas Grünbacher <andreas.gruenbacher@gmail.com> > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > After the merge window (and my vacation) I'll have the time to > re-introduce/work security xattr support. > > Thanks, > //richard > --- > fs/ubifs/super.c | 1 - > fs/ubifs/ubifs.h | 1 - > fs/ubifs/xattr.c | 40 ---------------------------------------- > 3 files changed, 42 deletions(-) > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c > index 9547a278..c71edca 100644 > --- a/fs/ubifs/super.c > +++ b/fs/ubifs/super.c > @@ -2037,7 +2037,6 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent) > if (c->max_inode_sz > MAX_LFS_FILESIZE) > sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE; > sb->s_op = &ubifs_super_operations; > - sb->s_xattr = ubifs_xattr_handlers; > > mutex_lock(&c->umount_mutex); > err = mount_ubifs(c); > diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h > index de75902..33b6ee7 100644 > --- a/fs/ubifs/ubifs.h > +++ b/fs/ubifs/ubifs.h > @@ -1470,7 +1470,6 @@ extern spinlock_t ubifs_infos_lock; > extern atomic_long_t ubifs_clean_zn_cnt; > extern struct kmem_cache *ubifs_inode_slab; > extern const struct super_operations ubifs_super_operations; > -extern const struct xattr_handler *ubifs_xattr_handlers[]; > extern const struct address_space_operations ubifs_file_address_operations; > extern const struct file_operations ubifs_file_operations; > extern const struct inode_operations ubifs_file_inode_operations; > diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c > index 96f3448..b512b14 100644 > --- a/fs/ubifs/xattr.c > +++ b/fs/ubifs/xattr.c > @@ -582,46 +582,6 @@ out_free: > return err; > } > > -static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, > - const char *name, size_t name_len, int flags) > -{ > - const int prefix_len = XATTR_SECURITY_PREFIX_LEN; > - const size_t total_len = prefix_len + name_len + 1; > - > - if (list && total_len <= list_size) { > - memcpy(list, XATTR_SECURITY_PREFIX, prefix_len); > - memcpy(list + prefix_len, name, name_len); > - list[prefix_len + name_len] = '\0'; > - } > - > - return total_len; > -} > - > -static int security_getxattr(struct dentry *d, const char *name, void *buffer, > - size_t size, int flags) > -{ > - return ubifs_getxattr(d, name, buffer, size); > -} > - > -static int security_setxattr(struct dentry *d, const char *name, > - const void *value, size_t size, int flags, > - int handler_flags) > -{ > - return ubifs_setxattr(d, name, value, size, flags); > -} > - > -static const struct xattr_handler ubifs_xattr_security_handler = { > - .prefix = XATTR_SECURITY_PREFIX, > - .list = security_listxattr, > - .get = security_getxattr, > - .set = security_setxattr, > -}; > - > -const struct xattr_handler *ubifs_xattr_handlers[] = { > - &ubifs_xattr_security_handler, > - NULL, > -}; > - > static int init_xattrs(struct inode *inode, const struct xattr *xattr_array, > void *fs_info) > { > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-08-20 at 10:48 +0800, Dongsheng Yang wrote: > On 08/20/2015 04:35 AM, Richard Weinberger wrote: > > This is a partial revert of commit > > d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53 > > ("UBIFS: Add security.* XATTR support for the UBIFS"). > > Hi Richard, > What about a full reverting of this commit. In ubifs, we > *can* support any namespace of xattr including user, trusted, > security > or other anyone prefixed by any words. But we have a > check_namespace() > in xattr.c to limit what we want to support. That said, if we want to > "Add security.* XATTR support for the UBIFS", what we need to do is > just extending the check_namespace() to allow security namespace > pass. > And yes, check_namespace() have been supporting security namespace. > > So, IMHO, we do not depend on the generic mechanism at all, and we > can > fully revert this commit. We just weren't careful enough.
Am 20.08.2015 um 04:48 schrieb Dongsheng Yang: > On 08/20/2015 04:35 AM, Richard Weinberger wrote: >> This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53 >> ("UBIFS: Add security.* XATTR support for the UBIFS"). > > Hi Richard, > What about a full reverting of this commit. In ubifs, we > *can* support any namespace of xattr including user, trusted, security > or other anyone prefixed by any words. But we have a check_namespace() > in xattr.c to limit what we want to support. That said, if we want to > "Add security.* XATTR support for the UBIFS", what we need to do is > just extending the check_namespace() to allow security namespace pass. > And yes, check_namespace() have been supporting security namespace. You're right. I thought/hoped we can re-use some parts of it. Se let's do a full revert. I'll send a v2 patch in a jiffy. > So, IMHO, we do not depend on the generic mechanism at all, and we can > fully revert this commit. > > But strange to me, why we picked this commit for ubifs? Artem, is there > something I am missing? TBH, I fear nobody noticed. :( Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 20, 2015 at 10:48:38AM +0800, Dongsheng Yang wrote: > On 08/20/2015 04:35 AM, Richard Weinberger wrote: > >This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53 > >("UBIFS: Add security.* XATTR support for the UBIFS"). > > Hi Richard, > What about a full reverting of this commit. In ubifs, we > *can* support any namespace of xattr including user, trusted, security > or other anyone prefixed by any words. But we have a check_namespace() > in xattr.c to limit what we want to support. That said, if we want to > "Add security.* XATTR support for the UBIFS", what we need to do is > just extending the check_namespace() to allow security namespace pass. > And yes, check_namespace() have been supporting security namespace. Is this good enough? Yes, it'd mean that the xattrs end up on disk, but then who's responsible for invoking the selected LSMs inode_init_security() hooks? AFAICT, we'd still need to invoke security_inode_init_security for newly created inodes (which, Richard's proposed commit still does). Thanks, Josh (who, admittedly, is neither a filesystem nor security module guy :)
On 08/26/2015 10:15 PM, Josh Cartwright wrote: > On Thu, Aug 20, 2015 at 10:48:38AM +0800, Dongsheng Yang wrote: >> On 08/20/2015 04:35 AM, Richard Weinberger wrote: >>> This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53 >>> ("UBIFS: Add security.* XATTR support for the UBIFS"). >> >> Hi Richard, >> What about a full reverting of this commit. In ubifs, we >> *can* support any namespace of xattr including user, trusted, security >> or other anyone prefixed by any words. But we have a check_namespace() >> in xattr.c to limit what we want to support. That said, if we want to >> "Add security.* XATTR support for the UBIFS", what we need to do is >> just extending the check_namespace() to allow security namespace pass. >> And yes, check_namespace() have been supporting security namespace. > > Is this good enough? Yes, it'd mean that the xattrs end up on disk, but > then who's responsible for invoking the selected LSMs inode_init_security() hooks? > AFAICT, we'd still need to invoke security_inode_init_security for newly > created inodes (which, Richard's proposed commit still does). OH, right. My bad!!!! I missed the security_inode_init_security(). Besides to allow security.* prefix in xattr, we still need to call security_inode_init_security() in ubifs_create(). That's true. So what we need to remove is only the ubifs_xattr_handlers. Thanx Josh, you are right. And Richard, sorry for my bad mind. Reviewed-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> Thanx Yang > > Thanks, > > Josh (who, admittedly, is neither a filesystem nor security module guy :) > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 9547a278..c71edca 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -2037,7 +2037,6 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent) if (c->max_inode_sz > MAX_LFS_FILESIZE) sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE; sb->s_op = &ubifs_super_operations; - sb->s_xattr = ubifs_xattr_handlers; mutex_lock(&c->umount_mutex); err = mount_ubifs(c); diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index de75902..33b6ee7 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -1470,7 +1470,6 @@ extern spinlock_t ubifs_infos_lock; extern atomic_long_t ubifs_clean_zn_cnt; extern struct kmem_cache *ubifs_inode_slab; extern const struct super_operations ubifs_super_operations; -extern const struct xattr_handler *ubifs_xattr_handlers[]; extern const struct address_space_operations ubifs_file_address_operations; extern const struct file_operations ubifs_file_operations; extern const struct inode_operations ubifs_file_inode_operations; diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c index 96f3448..b512b14 100644 --- a/fs/ubifs/xattr.c +++ b/fs/ubifs/xattr.c @@ -582,46 +582,6 @@ out_free: return err; } -static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, - const char *name, size_t name_len, int flags) -{ - const int prefix_len = XATTR_SECURITY_PREFIX_LEN; - const size_t total_len = prefix_len + name_len + 1; - - if (list && total_len <= list_size) { - memcpy(list, XATTR_SECURITY_PREFIX, prefix_len); - memcpy(list + prefix_len, name, name_len); - list[prefix_len + name_len] = '\0'; - } - - return total_len; -} - -static int security_getxattr(struct dentry *d, const char *name, void *buffer, - size_t size, int flags) -{ - return ubifs_getxattr(d, name, buffer, size); -} - -static int security_setxattr(struct dentry *d, const char *name, - const void *value, size_t size, int flags, - int handler_flags) -{ - return ubifs_setxattr(d, name, value, size, flags); -} - -static const struct xattr_handler ubifs_xattr_security_handler = { - .prefix = XATTR_SECURITY_PREFIX, - .list = security_listxattr, - .get = security_getxattr, - .set = security_setxattr, -}; - -const struct xattr_handler *ubifs_xattr_handlers[] = { - &ubifs_xattr_security_handler, - NULL, -}; - static int init_xattrs(struct inode *inode, const struct xattr *xattr_array, void *fs_info) {
This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53 ("UBIFS: Add security.* XATTR support for the UBIFS"). As UBIFS does not use generic inode xattr inode operations the code behind sb->s_xattr is never called. Remove that dead code for now. Cc: Subodh Nijsure <snijsure@grid-net.com> Cc: Marc Kleine-Budde <mkl@pengutronix.de> Cc: Brad Mouring <brad.mouring@ni.com> Cc: Gratian Crisan <gratian.crisan@ni.com> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com> Reported-by: Andreas Grünbacher <andreas.gruenbacher@gmail.com> Signed-off-by: Richard Weinberger <richard@nod.at> --- After the merge window (and my vacation) I'll have the time to re-introduce/work security xattr support. Thanks, //richard --- fs/ubifs/super.c | 1 - fs/ubifs/ubifs.h | 1 - fs/ubifs/xattr.c | 40 ---------------------------------------- 3 files changed, 42 deletions(-)