From patchwork Mon Apr 9 06:40:00 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Subodh Nijsure X-Patchwork-Id: 151409 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 5C6F2B7011 for ; Mon, 9 Apr 2012 16:41:27 +1000 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SH8Gm-0003rH-3k; Mon, 09 Apr 2012 06:40:08 +0000 Received: from mail-wi0-f171.google.com ([209.85.212.171]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1SH8Gi-0003r3-MA for linux-mtd@lists.infradead.org; Mon, 09 Apr 2012 06:40:06 +0000 Received: by wibhj13 with SMTP id hj13so1529412wib.0 for ; Sun, 08 Apr 2012 23:40:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=QaiQ8830lAoVEp744xyGwGNycbbAanZRGfB1iGh8/Kw=; b=e6omhW3MlLzyDr54+ZFSeX4MDEogq8Kpexspy5Z+UyQXDgFB3/WdXsrJ0YCEt5kLoC 9Wkp9akqRg0psdiN9Kn0qP5/vkfKPtY2Vsz5ucJuKgm4usvogWGi/2K5edYXoJplSfNR DQd4nhalJA7JCuZatMSwuVqKB8SSU69UU8ERU2x9bnL2w6/gNVfzLSJ6/vhYxZL3tlQp BQ75gFkAwUaZ4Zr88Lot/687yyq2lualGp0tTdNUAh8I9keskqACvx/DGZp05i/26c1S T7brjoYAJcTDiLj6mf/XrrhxtTLCYdWHzvblE2Ll3Nv0xUfILb/7ctN/f2it3Q4ykVvc 1zZQ== MIME-Version: 1.0 Received: by 10.216.132.202 with SMTP id o52mr3480954wei.106.1333953600242; Sun, 08 Apr 2012 23:40:00 -0700 (PDT) Received: by 10.180.145.193 with HTTP; Sun, 8 Apr 2012 23:40:00 -0700 (PDT) In-Reply-To: <4F824B4F.8090107@schaufler-ca.com> References: <1333892851-10758-1-git-send-email-snijsure@grid-net.com> <4F824B4F.8090107@schaufler-ca.com> Date: Sun, 8 Apr 2012 23:40:00 -0700 Message-ID: Subject: Re: [PATCH] Add security.selinux XATTR support for the UBIFS. Also fix couple of bugs in UBIFS extended attribute length calculation. From: Subodh Nijsure To: Casey Schaufler X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.7 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (subodh.nijsure[at]gmail.com) -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.212.171 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: Subodh Nijsure , Artem Bityutskiy , linux-kernel@vger.kernel.org, Adrian Hunter , LSM , linux-mtd@lists.infradead.org, SE Linux X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org On Sun, Apr 8, 2012 at 7:37 PM, Casey Schaufler wrote: > On 4/8/2012 6:47 AM, Subodh Nijsure wrote: >> TESTING: Tested on  MX28 based platforms using Micron MT29F2G08ABAEAH4 NAND >>          With these change we are able to label UBIFS filesystem with security.selinux >>          and run system with selinux enabled. >>          Also ran integck test on UBI filesystem. >> >> Signed-off-by: Subodh Nijsure > > There is no reason to restrict xattr support to SELinux. > SELinux specific behavior belongs within the SELinux LSM. > This code needs to support the full xattr semantics. > > Nacked-by: Casey Schaufler > >> --- >>  fs/ubifs/dir.c     |    4 ++ >>  fs/ubifs/file.c    |    6 ++ >>  fs/ubifs/journal.c |   12 +++- >>  fs/ubifs/super.c   |    3 + >>  fs/ubifs/ubifs.h   |    9 +++ >>  fs/ubifs/xattr.c   |  147 ++++++++++++++++++++++++++++++++++++++++++++++++---- >>  6 files changed, 167 insertions(+), 14 deletions(-) >> >> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c >> index ec9f187..f4e06c4 100644 >> --- a/fs/ubifs/dir.c >> +++ b/fs/ubifs/dir.c >> @@ -293,6 +293,7 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode, >>       ubifs_release_budget(c, &req); >>       insert_inode_hash(inode); >>       d_instantiate(dentry, inode); >> +     ubifs_init_security(dir, inode, &dentry->d_name); >>       return 0; >> >>  out_cancel: >> @@ -754,6 +755,7 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) >> >>       ubifs_release_budget(c, &req); >>       d_instantiate(dentry, inode); >> +     ubifs_init_security(dir, inode, &dentry->d_name); >>       return 0; >> >>  out_cancel: >> @@ -831,6 +833,7 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry, >>       ubifs_release_budget(c, &req); >>       insert_inode_hash(inode); >>       d_instantiate(dentry, inode); >> +     ubifs_init_security(dir, inode, &dentry->d_name); >>       return 0; >> >>  out_cancel: >> @@ -907,6 +910,7 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry, >>       ubifs_release_budget(c, &req); >>       insert_inode_hash(inode); >>       d_instantiate(dentry, inode); >> +     ubifs_init_security(dir, inode, &dentry->d_name); >>       return 0; >> >>  out_cancel: >> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c >> index 5c8f6dc..b8e9d66 100644 >> --- a/fs/ubifs/file.c >> +++ b/fs/ubifs/file.c >> @@ -1575,6 +1575,12 @@ const struct inode_operations ubifs_symlink_inode_operations = { >>       .follow_link = ubifs_follow_link, >>       .setattr     = ubifs_setattr, >>       .getattr     = ubifs_getattr, >> +#ifdef CONFIG_UBIFS_FS_XATTR >> +     .setxattr    = ubifs_symlink_setxattr, >> +     .getxattr    = ubifs_symlink_getxattr, >> +     .listxattr   = ubifs_listxattr, >> +     .removexattr = ubifs_removexattr, >> +#endif >>  }; >> >>  const struct file_operations ubifs_file_operations = { >> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c >> index 2f438ab..725dc17 100644 >> --- a/fs/ubifs/journal.c >> +++ b/fs/ubifs/journal.c >> @@ -553,7 +553,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir, >> >>       dbg_jnl("ino %lu, dent '%.*s', data len %d in dir ino %lu", >>               inode->i_ino, nm->len, nm->name, ui->data_len, dir->i_ino); >> -     ubifs_assert(dir_ui->data_len == 0); >> +     if (!xent) >> +             ubifs_assert(dir_ui->data_len == 0); >>       ubifs_assert(mutex_is_locked(&dir_ui->ui_mutex)); >> >>       dlen = UBIFS_DENT_NODE_SZ + nm->len + 1; >> @@ -572,7 +573,11 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir, >> >>       aligned_dlen = ALIGN(dlen, 8); >>       aligned_ilen = ALIGN(ilen, 8); >> -     len = aligned_dlen + aligned_ilen + UBIFS_INO_NODE_SZ; >> +     /* Make sure to account for dir_ui+data_len in length calculation >> +      * in case there is extended attribute. >> +      */ >> +     len = aligned_dlen + aligned_ilen + >> +           UBIFS_INO_NODE_SZ + dir_ui->data_len; >>       dent = kmalloc(len, GFP_NOFS); >>       if (!dent) >>               return -ENOMEM; >> @@ -649,7 +654,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir, >> >>       ino_key_init(c, &ino_key, dir->i_ino); >>       ino_offs += aligned_ilen; >> -     err = ubifs_tnc_add(c, &ino_key, lnum, ino_offs, UBIFS_INO_NODE_SZ); >> +     err = ubifs_tnc_add(c, &ino_key, lnum, ino_offs, >> +                         UBIFS_INO_NODE_SZ + dir_ui->data_len); >>       if (err) >>               goto out_ro; >> >> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c >> index 76e4e05..c28ac04 100644 >> --- a/fs/ubifs/super.c >> +++ b/fs/ubifs/super.c >> @@ -2061,6 +2061,9 @@ 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; >> +#ifdef CONFIG_UBIFS_FS_XATTR >> +     sb->s_xattr = ubifs_xattr_handlers; >> +#endif >> >>       mutex_lock(&c->umount_mutex); >>       err = mount_ubifs(c); >> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h >> index 93d59ac..60b57f7 100644 >> --- a/fs/ubifs/ubifs.h >> +++ b/fs/ubifs/ubifs.h >> @@ -36,6 +36,7 @@ >>  #include >>  #include >>  #include >> +#include >>  #include "ubifs-media.h" >> >>  /* Version of this UBIFS implementation */ >> @@ -1454,6 +1455,7 @@ 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; >> @@ -1736,10 +1738,17 @@ int ubifs_getattr(struct vfsmount *mnt, struct dentry *dentry, >>                 struct kstat *stat); >> >>  /* xattr.c */ >> + >>  int ubifs_setxattr(struct dentry *dentry, const char *name, >>                  const void *value, size_t size, int flags); >> +int ubifs_init_security(struct inode *dentry, struct inode *inode, >> +                     const struct qstr *qstr); >> +int ubifs_symlink_setxattr(struct dentry *dentry, const char *name, >> +                        const void *value, size_t size, int flags); >>  ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf, >>                      size_t size); >> +ssize_t ubifs_symlink_getxattr(struct dentry *dentry, const char *name, >> +                            void *buf, size_t size); >>  ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size); >>  int ubifs_removexattr(struct dentry *dentry, const char *name); >> >> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c >> index 85b2722..a402c7d 100644 >> --- a/fs/ubifs/xattr.c >> +++ b/fs/ubifs/xattr.c >> @@ -209,11 +209,11 @@ static int change_xattr(struct ubifs_info *c, struct inode *host, >>               goto out_free; >>       } >>       inode->i_size = ui->ui_size = size; >> -     ui->data_len = size; >> >>       mutex_lock(&host_ui->ui_mutex); >>       host->i_ctime = ubifs_current_time(host); >>       host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); >> +     ui->data_len = size; >>       host_ui->xattr_size += CALC_XATTR_BYTES(size); >> >>       /* >> @@ -293,18 +293,16 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum) >>       return ERR_PTR(-EINVAL); >>  } >> >> -int ubifs_setxattr(struct dentry *dentry, const char *name, >> -                const void *value, size_t size, int flags) >> +static int __ubifs_setxattr(struct inode *host, const char *name, >> +                         const void *value, size_t size, int flags) >>  { >> -     struct inode *inode, *host = dentry->d_inode; >> +     struct inode *inode; >>       struct ubifs_info *c = host->i_sb->s_fs_info; >>       struct qstr nm = { .name = name, .len = strlen(name) }; >>       struct ubifs_dent_node *xent; >>       union ubifs_key key; >>       int err, type; >> >> -     dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name, >> -             host->i_ino, dentry->d_name.len, dentry->d_name.name, size); >>       ubifs_assert(mutex_is_locked(&host->i_mutex)); >> >>       if (size > UBIFS_MAX_INO_DATA) >> @@ -356,10 +354,29 @@ out_free: >>       return err; >>  } >> >> -ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf, >> -                    size_t size) >> +int ubifs_symlink_setxattr(struct dentry *dentry, const char *name, >> +                        const void *value, size_t size, int flags) >>  { >> -     struct inode *inode, *host = dentry->d_inode; >> +     struct inode *host = dentry->d_inode; >> +     dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name, >> +             host->i_ino, dentry->d_name.len, dentry->d_name.name, size); >> +     return __ubifs_setxattr(host, name, value, size, flags); >> +} >> + >> +int ubifs_setxattr(struct dentry *dentry, const char *name, >> +                const void *value, size_t size, int flags) >> +{ >> +     struct inode *host = dentry->d_inode; >> +     dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name, >> +             host->i_ino, dentry->d_name.len, dentry->d_name.name, size); >> +     return __ubifs_setxattr(host, name, value, size, flags); >> +} >> + >> +static >> +ssize_t __ubifs_getxattr(struct inode *host, const char *name, void *buf, >> +                      size_t size) >> +{ >> +     struct inode *inode; >>       struct ubifs_info *c = host->i_sb->s_fs_info; >>       struct qstr nm = { .name = name, .len = strlen(name) }; >>       struct ubifs_inode *ui; >> @@ -367,8 +384,8 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf, >>       union ubifs_key key; >>       int err; >> >> -     dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name, >> -             host->i_ino, dentry->d_name.len, dentry->d_name.name, size); >> +     dbg_gen("xattr '%s', ino %lu  buf size %zd", name, >> +             host->i_ino, size); >> >>       err = check_namespace(&nm); >>       if (err < 0) >> @@ -416,6 +433,25 @@ out_unlock: >>       return err; >>  } >> >> +ssize_t ubifs_symlink_getxattr(struct dentry *dentry, const char *name, >> +                            void *buf, size_t size) >> +{ >> +     struct inode *host = dentry->d_inode; >> +     dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name, >> +             host->i_ino, dentry->d_name.len, dentry->d_name.name, size); >> +     return __ubifs_getxattr(host, name, buf, size); >> +} >> + >> +ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf, >> +                    size_t size) >> +{ >> +     struct inode *host = dentry->d_inode; >> +     dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name, >> +             host->i_ino, dentry->d_name.len, dentry->d_name.name, size); >> +     return __ubifs_getxattr(host, name, buf, size); >> +} >> + >> + >>  ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) >>  { >>       union ubifs_key key; >> @@ -568,3 +604,92 @@ out_free: >>       kfree(xent); >>       return err; >>  } >> + >> +size_t >> +ubifs_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; >> +} >> + >> + >> +int ubifs_security_getxattr(struct dentry *d, const char *name, >> +                         void *buffer, size_t size, int flags) >> +{ >> +     if (strcmp(name, "") == 0) >> +             return -EINVAL; >> +     return __ubifs_getxattr(d->d_inode, XATTR_NAME_SELINUX, buffer, size); >> +} >> + >> + >> +int ubifs_security_setxattr(struct dentry *d, const char *name, >> +                         const void *value, size_t size, >> +                         int flags, int handler_flags) >> +{ >> +     if (strcmp(name, "") == 0) >> +             return -EINVAL; >> +     return __ubifs_setxattr(d->d_inode, XATTR_NAME_SELINUX, value, > > This is silly. If you're supporting xattrs there is no reason > single out a particular LSM. Make this general so that Smack > and any other LSM that wants to use xattrs will work. > > Move the changes into the SELinux LSM if you are going to > hard code it. > Yup, my bad, following change fixes that specific issue and one can set xattr for all security.* namespace. Will submit patch v2 shortly... @@ -635,7 +635,7 @@ int ubifs_security_setxattr(struct dentry *d, const char *name, { if (strcmp(name, "") == 0) return -EINVAL; - return __ubifs_setxattr(d->d_inode, XATTR_NAME_SELINUX, value, + return __ubifs_setxattr(d->d_inode, name, value, size, flags); } diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c index a402c7d..c8be7cd 100644 --- a/fs/ubifs/xattr.c +++ b/fs/ubifs/xattr.c @@ -625,7 +625,7 @@ int ubifs_security_getxattr(struct dentry *d, const char *name, { if (strcmp(name, "") == 0) return -EINVAL; - return __ubifs_getxattr(d->d_inode, XATTR_NAME_SELINUX, buffer, size); + return __ubifs_getxattr(d->d_inode, name, buffer, size); }