LinuxLists.cc - [PATCH 0/4] UBIFS: add xattr support for security / SELinux

2014-10-31 19:43:09

Subject: [PATCH 0/4] UBIFS: add xattr support for security / SELinux

I'm reposting the patch series for security xattr / SELinux support on UBIFS
from Subodh Nijsure and Marc Kleine-Budde [1] in order to restart the process
of getting this support upstream.

Notes:

- I removed 'UBIFS: xattr: protect ui_size and data_len by ui_mutex' because
after looking through the comments before the definition of struct
ubifs_inode, I'm not sure what this was intended to fix. It looks like
i_size and data_len are not intended to be protected by ui_mutex, and I'm
unclear on why ui->ui_size needs to be protected here by host_ui's ui_mutex.
CCing Marc -- could you comment on how this is supposed to work?

- I made the suggested locking fixes in [2], with the exception of removing the
i_mutex lock/unlock around the call to security_inode_init_security(), which
caused an assert. With these fixes, I turned on lockdep and ran with SELinux
enabled on an ARM-based embedded target using UBIFS, and I saw no lockdep
warnings during filesystem labeling and normal operation.

[1] http://lists.infradead.org/pipermail/linux-mtd/2013-February/045794.html
[2] http://lists.infradead.org/pipermail/linux-mtd/2013-February/045871.html

Subodh Nijsure (4):
UBIFS: fix a couple bugs in UBIFS xattr length calculation
UBIFS: Add xattr support for symlinks
UBIFS: Add security.* XATTR support for the UBIFS
UBIFS: add ubifs_err() to print error reason

fs/ubifs/dir.c | 20 +++++++++
fs/ubifs/file.c | 4 ++
fs/ubifs/journal.c | 11 ++++-
fs/ubifs/super.c | 1 +
fs/ubifs/ubifs.h | 4 ++
fs/ubifs/xattr.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++-----
6 files changed, 145 insertions(+), 11 deletions(-)

--
2.1.1

2014-10-31 19:28:14

by Ben Shelton

[permalink] [raw]

Subject: [PATCH 2/4] UBIFS: Add xattr support for symlinks

2014-10-31 19:35:53

by Ben Shelton

[permalink] [raw]

Subject: [PATCH 3/4] UBIFS: Add security.* XATTR support for the UBIFS

From: Subodh Nijsure <[email protected]>

Signed-off-by: Subodh Nijsure <[email protected]>
Signed-off-by: Marc Kleine-Budde <[email protected]>
Signed-off-by: Ben Shelton <[email protected]>
Acked-by: Brad Mouring <[email protected]>
Acked-by: Terry Wilcox <[email protected]>
Acked-by: Gratian Crisan <[email protected]>
---
fs/ubifs/dir.c | 20 ++++++++++++++
fs/ubifs/super.c | 1 +
fs/ubifs/ubifs.h | 4 +++
fs/ubifs/xattr.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 104 insertions(+)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index ea41649..31de1c4 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -272,6 +272,10 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
goto out_budg;
}

+ err = ubifs_init_security(dir, inode, &dentry->d_name);
+ if (err)
+ goto out_cancel;
+
mutex_lock(&dir_ui->ui_mutex);
dir->i_size += sz_change;
dir_ui->ui_size = dir->i_size;
@@ -279,6 +283,7 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
if (err)
goto out_cancel;
+
mutex_unlock(&dir_ui->ui_mutex);

ubifs_release_budget(c, &req);
@@ -728,6 +733,10 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
goto out_budg;
}

+ err = ubifs_init_security(dir, inode, &dentry->d_name);
+ if (err)
+ goto out_cancel;
+
mutex_lock(&dir_ui->ui_mutex);
insert_inode_hash(inode);
inc_nlink(inode);
@@ -740,6 +749,7 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
ubifs_err("cannot create directory, error %d", err);
goto out_cancel;
}
+
mutex_unlock(&dir_ui->ui_mutex);

ubifs_release_budget(c, &req);
@@ -808,6 +818,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
ui->data = dev;
ui->data_len = devlen;

+ err = ubifs_init_security(dir, inode, &dentry->d_name);
+ if (err)
+ goto out_cancel;
+
mutex_lock(&dir_ui->ui_mutex);
dir->i_size += sz_change;
dir_ui->ui_size = dir->i_size;
@@ -815,6 +829,7 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
if (err)
goto out_cancel;
+
mutex_unlock(&dir_ui->ui_mutex);

ubifs_release_budget(c, &req);
@@ -884,6 +899,10 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
ui->data_len = len;
inode->i_size = ubifs_inode(inode)->ui_size = len;

+ err = ubifs_init_security(dir, inode, &dentry->d_name);
+ if (err)
+ goto out_cancel;
+
mutex_lock(&dir_ui->ui_mutex);
dir->i_size += sz_change;
dir_ui->ui_size = dir->i_size;
@@ -891,6 +910,7 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
if (err)
goto out_cancel;
+
mutex_unlock(&dir_ui->ui_mutex);

ubifs_release_budget(c, &req);
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 106bf20..e642067 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2039,6 +2039,7 @@ 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 c4fe900..bc04b9c 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -36,6 +36,7 @@
#include <linux/mtd/ubi.h>
#include <linux/pagemap.h>
#include <linux/backing-dev.h>
+#include <linux/security.h>
#include "ubifs-media.h"

/* Version of this UBIFS implementation */
@@ -1465,6 +1466,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;
@@ -1754,6 +1756,8 @@ ssize_t ubifs_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);
+int ubifs_init_security(struct inode *dentry, struct inode *inode,
+ const struct qstr *qstr);

/* super.c */
struct inode *ubifs_iget(struct super_block *sb, unsigned long inum);
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index d0f69c2..f844841 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -575,3 +575,82 @@ 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)
+{
+ return ubifs_getxattr(d, name, buffer, size);
+}
+
+int ubifs_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);
+}
+
+struct xattr_handler ubifs_xattr_security_handler = {
+ .prefix = XATTR_SECURITY_PREFIX,
+ .list = ubifs_security_listxattr,
+ .get = ubifs_security_getxattr,
+ .set = ubifs_security_setxattr,
+};
+
+const struct xattr_handler *ubifs_xattr_handlers[] = {
+ &ubifs_xattr_security_handler,
+ NULL
+};
+
+static int ubifs_initxattrs(struct inode *inode,
+ const struct xattr *xattr_array, void *fs_info)
+{
+ const struct xattr *xattr;
+ char *name;
+ int err = 0;
+
+ for (xattr = xattr_array; xattr->name != NULL; xattr++) {
+ name = kmalloc(XATTR_SECURITY_PREFIX_LEN +
+ strlen(xattr->name) + 1, GFP_NOFS);
+ if (!name) {
+ err = -ENOMEM;
+ break;
+ }
+ strcpy(name, XATTR_SECURITY_PREFIX);
+ strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
+ err = __ubifs_setxattr(inode, name, xattr->value,
+ xattr->value_len, 0);
+ kfree(name);
+ if (err < 0)
+ break;
+ }
+
+ return err;
+}
+
+int ubifs_init_security(struct inode *dentry, struct inode *inode,
+ const struct qstr *qstr)
+{
+ int err;
+
+ mutex_lock(&inode->i_mutex);
+ err = security_inode_init_security(inode, dentry, qstr,
+ &ubifs_initxattrs, 0);
+ mutex_unlock(&inode->i_mutex);
+
+ return err;
+}
--
2.1.1

2014-10-31 19:38:37

by Ben Shelton

[permalink] [raw]

Subject: [PATCH 1/4] UBIFS: fix a couple bugs in UBIFS xattr length calculation

2014-10-31 19:57:40

by Ben Shelton

[permalink] [raw]

Subject: [PATCH 4/4] UBIFS: add ubifs_err() to print error reason

From: Subodh Nijsure <[email protected]>

This patch adds ubifs_err() output to some error paths to tell the user
what's going on.

Signed-off-by: Subodh Nijsure <[email protected]>
Signed-off-by: Marc Kleine-Budde <[email protected]>
Signed-off-by: Ben Shelton <[email protected]>
Acked-by: Brad Mouring <[email protected]>
Acked-by: Terry Wilcox <[email protected]>
Acked-by: Gratian Crisan <[email protected]>
---
fs/ubifs/xattr.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index f844841..2b99e49 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -100,24 +100,32 @@ static const struct file_operations empty_fops;
static int create_xattr(struct ubifs_info *c, struct inode *host,
const struct qstr *nm, const void *value, int size)
{
- int err;
+ int err, xattr_name_list_size;
struct inode *inode;
struct ubifs_inode *ui, *host_ui = ubifs_inode(host);
struct ubifs_budget_req req = { .new_ino = 1, .new_dent = 1,
.new_ino_d = ALIGN(size, 8), .dirtied_ino = 1,
.dirtied_ino_d = ALIGN(host_ui->data_len, 8) };

- if (host_ui->xattr_cnt >= MAX_XATTRS_PER_INODE)
+ if (host_ui->xattr_cnt >= MAX_XATTRS_PER_INODE) {
+ ubifs_err("ubifs xattr_cnt %d exceeds MAX_XATTR_PER_NODE (%d)",
+ host_ui->xattr_cnt, MAX_XATTRS_PER_INODE);
return -ENOSPC;
+ }
/*
* Linux limits the maximum size of the extended attribute names list
* to %XATTR_LIST_MAX. This means we should not allow creating more
* extended attributes if the name list becomes larger. This limitation
* is artificial for UBIFS, though.
*/
- if (host_ui->xattr_names + host_ui->xattr_cnt +
- nm->len + 1 > XATTR_LIST_MAX)
+ xattr_name_list_size = host_ui->xattr_names + host_ui->xattr_cnt +
+ nm->len + 1;
+
+ if (xattr_name_list_size > XATTR_LIST_MAX) {
+ ubifs_err("xattr name list too large %d > %d",
+ xattr_name_list_size, XATTR_LIST_MAX);
return -ENOSPC;
+ }

err = ubifs_budget_space(c, &req);
if (err)
@@ -652,5 +660,9 @@ int ubifs_init_security(struct inode *dentry, struct inode *inode,
&ubifs_initxattrs, 0);
mutex_unlock(&inode->i_mutex);

+ if (err)
+ ubifs_err("cannot initialize extended attribute, error %d",
+ err);
+
return err;
}
--
2.1.1

2014-11-07 09:53:59

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 0/4] UBIFS: add xattr support for security / SELinux

On Fri, 2014-10-31 at 13:50 -0500, Ben Shelton wrote:
> I'm reposting the patch series for security xattr / SELinux support on UBIFS
> from Subodh Nijsure and Marc Kleine-Budde [1] in order to restart the process
> of getting this support upstream.
>
> Notes:
>
> - I removed 'UBIFS: xattr: protect ui_size and data_len by ui_mutex' because
> after looking through the comments before the definition of struct
> ubifs_inode, I'm not sure what this was intended to fix. It looks like
> i_size and data_len are not intended to be protected by ui_mutex, and I'm
> unclear on why ui->ui_size needs to be protected here by host_ui's ui_mutex.
> CCing Marc -- could you comment on how this is supposed to work?

Thanks, I'll be looking at this as time allows. I think the MTD web site
mentioned somewhere that SELinux is not supported or something. In any
case, once we support SELinux, we need to tell on the web site that we
support it.

Which reminds me that I need to update the web-site and tell there that
I do not support the backport trees any longer...

2014-11-07 10:34:46

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 1/4] UBIFS: fix a couple bugs in UBIFS xattr length calculation

On Fri, 2014-10-31 at 13:50 -0500, Ben Shelton wrote:
> From: Subodh Nijsure <[email protected]>
>
> Signed-off-by: Subodh Nijsure <[email protected]>
> Signed-off-by: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Ben Shelton <[email protected]>
> Acked-by: Brad Mouring <[email protected]>
> Acked-by: Gratian Crisan <[email protected]>

It looks like these patches are against an old kernel. In the new kernel
"dir_ui" was renamed to "host_ui", to show that this is not just a
directory inode, but a directory or an xattr inode (we call the "host"
inodes, since the "host" the xattr value).

I've pushed this patch in a modified form.

Do you have a possibility to test this series with a newer kernel?

Here is the patch I pushed:

>From a76284e6f89b2ae37d413fe793752257be01765a Mon Sep 17 00:00:00 2001
From: Subodh Nijsure <[email protected]>
Date: Fri, 31 Oct 2014 13:50:28 -0500
Subject: [PATCH] UBIFS: fix a couple bugs in UBIFS xattr length calculation

The journal update function did not work for extended attributes properly,
because extended attribute inodes carry the xattr data, and the size of this
data was not taken into account.

Artem: improved commit message, amended the patch a bit.

Signed-off-by: Subodh Nijsure <[email protected]>
Signed-off-by: Marc Kleine-Budde <[email protected]>
Signed-off-by: Ben Shelton <[email protected]>
Acked-by: Brad Mouring <[email protected]>
Acked-by: Gratian Crisan <[email protected]>
Signed-off-by: Artem Bityutskiy <[email protected]>
---
fs/ubifs/journal.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index fb166e2..f6ac3f2 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -571,7 +571,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 also account for extended attributes */
+ len += host_ui->data_len;
+
dent = kmalloc(len, GFP_NOFS);
if (!dent)
return -ENOMEM;
@@ -648,7 +652,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 + host_ui->data_len);
if (err)
goto out_ro;

--
1.9.3

Artem.

2014-11-07 19:57:03

by Ben Shelton

[permalink] [raw]

Subject: Re: [PATCH 1/4] UBIFS: fix a couple bugs in UBIFS xattr length calculation

On 11/07, Artem Bityutskiy wrote:
> On Fri, 2014-10-31 at 13:50 -0500, Ben Shelton wrote:
> > From: Subodh Nijsure <[email protected]>
> >
> > Signed-off-by: Subodh Nijsure <[email protected]>
> > Signed-off-by: Marc Kleine-Budde <[email protected]>
> > Signed-off-by: Ben Shelton <[email protected]>
> > Acked-by: Brad Mouring <[email protected]>
> > Acked-by: Gratian Crisan <[email protected]>
>
> It looks like these patches are against an old kernel. In the new kernel
> "dir_ui" was renamed to "host_ui", to show that this is not just a
> directory inode, but a directory or an xattr inode (we call the "host"
> inodes, since the "host" the xattr value).
>
> I've pushed this patch in a modified form.
>
> Do you have a possibility to test this series with a newer kernel?

Thanks for having a look at the patches.

We originally tested the patches against a recent stable 3.14 kernel.
I'll try to get a more recent kernel running on one of our targets that
uses UBIFS to test with -- should I use 3.18-rc3 or the latest
l2-mtd.git master?

Ben

>
> Here is the patch I pushed:
>
>
> From a76284e6f89b2ae37d413fe793752257be01765a Mon Sep 17 00:00:00 2001
> From: Subodh Nijsure <[email protected]>
> Date: Fri, 31 Oct 2014 13:50:28 -0500
> Subject: [PATCH] UBIFS: fix a couple bugs in UBIFS xattr length calculation
>
> The journal update function did not work for extended attributes properly,
> because extended attribute inodes carry the xattr data, and the size of this
> data was not taken into account.
>
> Artem: improved commit message, amended the patch a bit.
>
> Signed-off-by: Subodh Nijsure <[email protected]>
> Signed-off-by: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Ben Shelton <[email protected]>
> Acked-by: Brad Mouring <[email protected]>
> Acked-by: Gratian Crisan <[email protected]>
> Signed-off-by: Artem Bityutskiy <[email protected]>
> ---
> fs/ubifs/journal.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> index fb166e2..f6ac3f2 100644
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -571,7 +571,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 also account for extended attributes */
> + len += host_ui->data_len;
> +
> dent = kmalloc(len, GFP_NOFS);
> if (!dent)
> return -ENOMEM;
> @@ -648,7 +652,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 + host_ui->data_len);
> if (err)
> goto out_ro;
>
> --
> 1.9.3
>
> Artem.
>

2014-11-10 14:02:03

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 2/4] UBIFS: Add xattr support for symlinks

2014-11-10 17:14:04

by Ben Shelton

[permalink] [raw]

Subject: Re: [PATCH 2/4] UBIFS: Add xattr support for symlinks

On 11/10, Artem Bityutskiy wrote:
> Could you please re-test this with any kernel and carefully verify
> symlinks. I think this should not work, because in case of symlinks we
> already store the link target path in the inode, and with this patch the
> target patch will be over-written with the SELinux label. I expect this
> to be seen easily on testing - symlink targets should be corrupted.
>
> Artem.
>

I retested this with a 3.18-rc3 kernel on one of our ARM-based targets.
The kernel has patch 1/4 with your changes, plus patches 2/4, 3/4, and
4/4 as posted.

Initially, I booted the target with SELinux disabled. I then created
'testfile' and made a symlink 'testlink' pointing to it. I also created
a symlink 'testlink_2' that points to /bin/bash.

I then enabled SELinux in permissive mode and rebooted the target. As
this was the first boot into SELinux, it relabeled the filesystems and
rebooted. After it came back up, I created 'testfile_afterrelabel' and
made a symlink 'testlink_afterrelabel' pointing to it. In addition, I
checked the symlinks that are managed by update-alternatives. Finally,
I ran `ls -lRZ / | grep ^l` and did not see any corrupted symlink
targets.

The results are below, and they look sane to me. Please let me know if
there is additional testing you would like me to perform.

admin@galvanized:~# uname -a
Linux galvanized 3.18.0-rc3-ni-04094-g7b78529 #1 SMP Mon Nov 10 09:59:06 CST 2014 armv7l GNU/Linux
admin@galvanized:~# mount | grep ubifs
ubi1:rootfs on / type ubifs (rw,relatime,seclabel)
ubi0:bootfs on /boot type ubifs (rw,noatime,sync,seclabel)
ubi0:config on /etc/natinst/share type ubifs (rw,relatime,sync,seclabel)
admin@galvanized:~# pwd
/home/admin
admin@galvanized:~# ls -lZ
total 8
-rw-r--r--. 1 admin administrators user_u:object_r:user_home_t 15 Nov 10 16:20 testfile
-rw-r--r--. 1 admin administrators root:object_r:user_home_t 21 Nov 10 16:50 testfile_afterrelabel
lrwxrwxrwx. 1 admin administrators user_u:object_r:user_home_t 8 Nov 10 16:21 testlink -> testfile
lrwxrwxrwx. 1 admin administrators user_u:object_r:user_home_t 9 Nov 10 16:21 testlink_2 -> /bin/bash
lrwxrwxrwx. 1 admin administrators root:object_r:user_home_t 21 Nov 10 16:51 testlink_afterrelabel -> testfile_afterrelabel
admin@galvanized:~# which ls
/bin/ls
admin@galvanized:~# ls -lZ /bin/ls
lrwxrwxrwx. 1 admin administrators system_u:object_r:bin_t 12 Nov 10 16:08 /bin/ls -> ls.coreutils
admin@galvanized:~# ls -lZ /bin/grep
lrwxrwxrwx. 1 admin administrators system_u:object_r:bin_t 25 Nov 5 20:39 /bin/grep -> /usr/lib/busybox/bin/grep

Best,
Ben

2014-11-11 10:17:58

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 2/4] UBIFS: Add xattr support for symlinks

On Mon, 2014-11-10 at 11:12 -0600, Ben Shelton wrote:
> On 11/10, Artem Bityutskiy wrote:
> > Could you please re-test this with any kernel and carefully verify
> > symlinks. I think this should not work, because in case of symlinks we
> > already store the link target path in the inode, and with this patch the
> > target patch will be over-written with the SELinux label. I expect this
> > to be seen easily on testing - symlink targets should be corrupted.
> >
> > Artem.
> >
>
> I retested this with a 3.18-rc3 kernel on one of our ARM-based targets.
> The kernel has patch 1/4 with your changes, plus patches 2/4, 3/4, and
> 4/4 as posted.

Ben, thanks for re-testing. And yes, I was wrong. Now I checked again
and finally remembered how it works.

So yes, symlink inode has data, and the target is in the data.

If we add an extattr to a symlink, we create a separate inode for that
xattr under the symlink inode. Any new xattr gets a new inode.

Artem.

2014-11-11 11:04:35

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 2/4] UBIFS: Add xattr support for symlinks

I've pushed this patch to the master branch. But I did ab amendment.

On Fri, 2014-10-31 at 13:50 -0500, Ben Shelton wrote:
> +static int __ubifs_setxattr(struct inode *host, const char *name,
> + const void *value, size_t size, int flags)
> {

I renamed this to just 'setxattr()', to be consistent wit the rest of
the code where we use the following convention:

* non-static functions start with 'ubifs_'
* static functions use any name, and do not start with 'ubifs_'
* we just do not prefix functions with '__' anywhere, so let's not use
it in xattr.c either.

Artem.

2014-11-11 11:07:49

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 3/4] UBIFS: Add security.* XATTR support for the UBIFS

Hi,

pushed this patch too, with amendments.

On Fri, 2014-10-31 at 13:50 -0500, Ben Shelton wrote:
> @@ -279,6 +283,7 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
> err = ubifs_jnl_update(c, dir, &dentry->d_name, inode, 0, 0);
> if (err)
> goto out_cancel;
> +
> mutex_unlock(&dir_ui->ui_mutex);

Removed this junk newline change here and in other similar places.
> +size_t ubifs_security_listxattr(struct dentry *d, char *list, size_t list_size,
> + const char *name, size_t name_len, int flags)

Made this to be static, renamed to 'security_listxattr()'.

> +int ubifs_security_getxattr(struct dentry *d, const char *name,
> + void *buffer, size_t size, int flags)

Similar: rename + static.

> +int ubifs_security_setxattr(struct dentry *d, const char *name,
> + const void *value, size_t size,
> + int flags, int handler_flags)
Similar: rename + static.

> +struct xattr_handler ubifs_xattr_security_handler = {

Renamed, made static and const too.

> +static int ubifs_initxattrs(struct inode *inode,
> + const struct xattr *xattr_array, void *fs_info)

Renamed to just init_xattrs(), for the consistency reasons, the ones I
explained in the review to the patch 2/4.

2014-11-11 11:40:40

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 4/4] UBIFS: add ubifs_err() to print error reason

Pushed this, but made some amendments.

Now all 4 are in the master branch. Would you please re-test them again
and confirm that they are fine. Then I'll push them to the linux-next
branch, unless you report that my amendments broke something.

Thank you!

On Fri, 2014-10-31 at 13:50 -0500, Ben Shelton wrote:
> @@ -100,24 +100,32 @@ static const struct file_operations empty_fops;
> static int create_xattr(struct ubifs_info *c, struct inode *host,
> const struct qstr *nm, const void *value, int size)
> {
> - int err;
> + int err, xattr_name_list_size;

Renamed this to a shorter names_len.

> - if (host_ui->xattr_cnt >= MAX_XATTRS_PER_INODE)
> + if (host_ui->xattr_cnt >= MAX_XATTRS_PER_INODE) {
> + ubifs_err("ubifs xattr_cnt %d exceeds MAX_XATTR_PER_NODE (%d)",
> + host_ui->xattr_cnt, MAX_XATTRS_PER_INODE);

Improved this message - made it more user-friendly, and print the inode
number.

> + if (xattr_name_list_size > XATTR_LIST_MAX) {
> + ubifs_err("xattr name list too large %d > %d",
> + xattr_name_list_size, XATTR_LIST_MAX);
> return -ENOSPC;

Ditto.
>
> + if (err)
> + ubifs_err("cannot initialize extended attribute, error %d",
> + err);

Refactored this message and added the inode number.

Artem.

2014-11-11 16:08:55

by Ben Shelton

[permalink] [raw]

Subject: Re: [PATCH 4/4] UBIFS: add ubifs_err() to print error reason

On 11/11, Artem Bityutskiy wrote:
> Pushed this, but made some amendments.
>
> Now all 4 are in the master branch. Would you please re-test them again
> and confirm that they are fine. Then I'll push them to the linux-next
> branch, unless you report that my amendments broke something.
>
> Thank you!
>

Hi Artem,

I pulled the four modified commits into my 3.18-rc3 branch and tested
with lockdep enabled. Everything looks good.

Thanks,
Ben

2014-11-12 12:31:35

by Artem Bityutskiy

[permalink] [raw]

Subject: Re: [PATCH 4/4] UBIFS: add ubifs_err() to print error reason

On Tue, 2014-11-11 at 10:08 -0600, Ben Shelton wrote:
> On 11/11, Artem Bityutskiy wrote:
> > Pushed this, but made some amendments.
> >
> > Now all 4 are in the master branch. Would you please re-test them again
> > and confirm that they are fine. Then I'll push them to the linux-next
> > branch, unless you report that my amendments broke something.
> >
> > Thank you!
> >
>
> Hi Artem,
>
> I pulled the four modified commits into my 3.18-rc3 branch and tested
> with lockdep enabled. Everything looks good.

OK, thank you!