Re: [PATCH resend v2] Add security.selinux XATTR support for the UBIFS. Also fix couple of bugs in UBIFS extended attribute storage

Re: [PATCH resend v2] Add security.selinux XATTR support for the UBIFS. Also fix couple of bugs in UBIFS extended attribute storage

[Bityutskiy, Artem]

[CCing fs-devel mailing list, please, keep it in CC]
[Please, CC fs-devel when you submit the next version]

Good job in general, thanks a lot. But I think you need to work on this
patch some more.

I remember long time ago you said you found a bug in UBIFS xattr
handling - could you please submit it as a separate patch?

First, few nitpicks:

On Fri, 2011-10-14 at 14:50 -0700, Subodh Nijsure wrote:
> Only change since the previous patch is source code format changes.

1. Why you removed the text about how you tested this with selinux?
   That text was included when you submitted the patch first time.
2. But please, do not put that big shell scripts to commit message :-)
3. Please, clean-up the subject - it should be shorter.
4. Please, add "UBIFS:" prefix to the subject.


> mkdir -p /tmp/flash0
> mount -t ubifs ubi0:root-0 /tmp/flash0
> integck -p /tmp/flash0/

5. Please, remove this script, it is enough to say that integck was
   happy.

6. Thanks for aligning the style with the original UBIFS style. However,
   this is not exactly it. You should you tabs for indents, and at the
   end use few spaces to fine-tune the alignment. You should never put
   more than 7 spaces, because 8 spaces is already one tab.

   Or to put it differently, run checkpatch.pl and you will see error -
   please, fix them.

> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 6834920..b744cd8 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -253,6 +253,52 @@ out:
>  	return ERR_PTR(err);
>  }
>  
> +static void ubifs_init_security(struct dentry *dentry, struct inode
> *inode,
> +				struct inode *dir)

Could you please send a patch which is not line-wrapped? Try 'git
send-mail'. Try to save this patch and apply it - you will see why I
complain: it won't apply. In general, you should send patches which
others can save and just apply with 'git am'.

> +{
> +	int err;
> +	char *name;
> +	void *value = NULL;
> +	size_t len = 0;
> +	struct ubifs_inode *dir_ui = ubifs_inode(dir);
> +	const struct qstr *qname = &dentry->d_name;
> +
> +	mutex_lock(&dir_ui->ui_mutex);
> +	mutex_lock(&dentry->d_inode->i_mutex);
> +	err = security_inode_init_security(inode, dir, qname, &name, &value,
> +		&len);

Please, align len to the "inode" using first tabs then few spaces, to be of
the same style as the rest of the code.

> +	if (err) {
> +		if (err == -EOPNOTSUPP)
> +
			return;
I believe you meant:

if (err == -EOPNOTSUPP)
	ubifs_err("unable to retrieve security context, error %d", err);

Or you really meant to return without unlocking the mutexes?

> +		ubifs_err("unable to retrieve security context, error %d", err);
> +		mutex_unlock(&dentry->d_inode->i_mutex);
> +		mutex_unlock(&dir_ui->ui_mutex);
> +		return;

Remove 3 above lines and put:

		goto out_unlock;

instead.

> +	}
> +
> +	if (strncmp(name, "selinux", strlen("selinux")) == 0) {
> +		kfree(name);
> +		name = kstrdup("security.selinux", GFP_NOFS);

Sorry, no, you should not re-allocate this, this is not a clean approach.
Please, take a look at ext4 and try to model after their code. Well,
they probably do more than you need, so do not copy everything blindly
of course.

In theory, this code should be the same for any LSM - whether it is
selinux or smack. You really should not have 'strcmp(name, "selinux")'
things.

Also, I strongly suspect that ext4 does not store the 'security.' part
on the media - but I did not check this for sure and leave this activity
to you :-) If it does not, we also should not do this.

> +		if (!name) {
> +			ubifs_err("unable to set security context %.*s error",
> +				  dentry->d_name.len, dentry->d_name.name);
> +			kfree(value);
> +			mutex_unlock(&dentry->d_inode->i_mutex);
> +			mutex_unlock(&dir_ui->ui_mutex);
> +			return;
> +		}
> +	}

So, taking into account what I said above, I think whole this block should
die.

Well, I personally do not have objections if you do not test smack.
Of course it would be great if you did, but I am fine if you put
something like this here and leave the smack exercise for someone
who uses it:

/* We tested only SElinux, sorry */
if (strcmp(name, XATTR_SELINUX_SUFFIX)
	goto out_free;


> +	err = ubifs_setxattr(dentry, name, value, len, 0);
> +	if (err)
> +		ubifs_err("unable to set security context name %.*s error %d",
> +			  dentry->d_name.len, dentry->d_name.name, err);

So, probably you need to pass the XATTR_SECURITY_PREFIX into this function
and let it deal with the name. Probably you can add one more parameter to
this function, say, "prefix".  For non-security cases it would be NULL,
otherwise - XATTR_SECURITY_PREFIX.

Please, make that to be a _separate_ patch.

> +	kfree(name);
out_free:
> +	kfree(value);
out_unlock:
> +	mutex_unlock(&dentry->d_inode->i_mutex);
> +	mutex_unlock(&dir_ui->ui_mutex);
> +}


-- 
Best Regards,
Artem Bityutskiy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.