Setting security XATTR on ubifs

Setting security XATTR on ubifs

[Subodh Nijsure]

we have implemented modifications to UBIFS to add support for SELinux
labeling. Function that created this XATTR is called
ubifs_init_security(), shown below.

Following example of how JFFS2 does extended attribute labeling, This
function is being called from 
ubifs_create(),ubifs_mkdir(), ubifs_mknod(), ubifs_symlink() (in
fs/ubifs/dir.c)

With this modification things work "mostly", I am able to label the file
system, but sometimes the file system is getting corrupted. I will
certainly post the patch once things work reliably. 

I don't _fully_ understand how ubifs is doing space management, hence
the immediate questions I have are:
1. What is the right point to add the XATTR to the UBIFS inode, after
the ubifs_new_inode() is done? Should ubifs_budget_space() be updated to
handle extra space needed by the XATTR.

2. In function below ui_mutex is being locked/unlocked while XATTR for
the file is updated. Is that required while updating the extended
attribute?

-Subodh Nijsure

static void ubifs_init_security(struct dentry *dentry, struct inode
*inode, struct inode *dir)
{
        int err;
        char *name;
        void *value = NULL;
        size_t len = 0;
        struct ubifs_inode *dir_ui = ubifs_inode(dir);
        mutex_lock(&dir_ui->ui_mutex);
        err = security_inode_init_security(inode, dir, &name, &value,
&len);
        if (err) {
                if (err == -EOPNOTSUPP)
                        return;
                ubifs_err("unable to retrieve security context, error %
d", err);
                mutex_unlock(&dir_ui->ui_mutex);
                return;
        }
        err = ubifs_setxattr(dentry, name, value, len, 0);
        if (err)
                ubifs_err("unable to set security context (extended
attribute), err %d",err);
        kfree(name);
        if ( value )
                kfree(value);
        mutex_unlock(&dir_ui->ui_mutex);
}

Re: Setting security XATTR on ubifs

[Artem Bityutskiy]

Hi,

no time now, so very quick answer.

On Fri, 2011-05-20 at 16:36 -0700, Subodh Nijsure wrote:
> we have implemented modifications to UBIFS to add support for SELinux
> labeling. Function that created this XATTR is called
> ubifs_init_security(), shown below.

OK, sounds cool.

> Following example of how JFFS2 does extended attribute labeling, This
> function is being called from 
> ubifs_create(),ubifs_mkdir(), ubifs_mknod(), ubifs_symlink() (in
> fs/ubifs/dir.c)

OK.

> With this modification things work "mostly", I am able to label the file
> system, but sometimes the file system is getting corrupted. I will
> certainly post the patch once things work reliably. 

Hmm... Running tests with all UBIFS extra self-checks turned on is a
good idea, BTW.

> I don't _fully_ understand how ubifs is doing space management, hence
> the immediate questions I have are:

Well, the main complication is budgeting: before doing any I/O you need
to get space budget, which is basically about asking the budgeting
subsystem to reserve certain amount of space for your operation. When
you are done - you release the budget.

> 1. What is the right point to add the XATTR to the UBIFS inode, after
> the ubifs_new_inode() is done?

I guess right after that? But keep in mind that unclean reboots may
result in losing selinux attributes. E.g., you create a new inode for
file XXX and this succeeds, then you start creating the selinux labels
and you have a power cut. When you mount the FS again you'll find out
that XXX does not have any selinux labels.

I do not know selinux well enough so do not know how this is handled:
does selinux handle it from user-space somehow or it assumes that
file/dir/symlink + selinux lables creation is atomic?

>  Should ubifs_budget_space() be updated to
> handle extra space needed by the XATTR.

In _your_ implementation no - ubifs_init_security() calls
'ubifs_setxattr()' which calls 'create_xattr()' which gets the budget.

I mean, in your implementation creating inode and creating selinux
xattrs are 2 separate operations and you use existing "big" building
blocks for this which do the budgeting.

> 2. In function below ui_mutex is being locked/unlocked while XATTR for
> the file is updated. Is that required while updating the extended
> attribute?

You'd deadlock if you did this I think, because 'create_xattr()' takes
the mutex (it is called "host_ui->ui_mutex" there).

HTH.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

RE: Setting security XATTR on ubifs

[Artem Bityutskiy]

On Fri, 2011-05-27 at 06:08 -0700, Subodh Nijsure wrote:
> Yup I did run UBIFS with debug & extra checks and discovered few issues.
> Found that setxattr/getxattr require that we should have already taken
> host->i_mutex. So I have updated my ubifs_init_security() accordingly.

Great!

> One of the problem turned out to be in change_xattr function, ui->data_len
> was updated at wrong place, and hence when after unmount, mount the
> length of the XATTR was wrong. I test
> 
> Diff below fixes that immediate issue. I tested this with
> As I mentioned, will post entire patch that allows ubifs to work with
> SELinux by end of this week.
> 
> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
> index 3299f46..a413278 100644
> --- a/fs/ubifs/xattr.c
> +++ b/fs/ubifs/xattr.c
> @@ -211,11 +211,11 @@ static int change_xattr(struct ubifs_info *c,
> struct inode *host,
>         }
>         memcpy(ui->data, value, size);
>         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);

Could you please submit a separate patch for this with

Cc: stable@kernel.org

> Now I am looking at much bigger issue where, if I create char device
> and assign it XATTR, on umount the file system is getting marked
> corrupted. This is with testing on NANDSIM, it happens on actual HW
> too. It appears that node_scan doesn't seem to be reading XATTR data
> and accounting for it when computing checksum.

If you send me a test program I might look at this, but no promises.
Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

RE: Setting security XATTR on ubifs

[Artem Bityutskiy]

On Mon, 2011-05-30 at 12:12 +0300, Artem Bityutskiy wrote:
> Could you please submit a separate patch for this with
> 
> Cc: stable@kernel.org

Hi, should I expect a patch?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)