Re: [PATCH v4] Add security.* XATTR support for the UBIFS

From: Artem Bityutskiy
Date: Tue May 15 2012 - 06:25:54 EST


On Mon, 2012-05-14 at 14:09 -0700, Subodh Nijsure wrote:
> On 05/14/2012 06:02 AM, Artem Bityutskiy wrote:
> > On Sun, 2012-05-13 at 06:24 -0700, snijsure@xxxxxxxxxxxx wrote:
> >> +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, name, 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;
> > Should this check pushed town to __ubifs_getxattr/__ubifs_setxattr ?
> If you really want to move that check into __ubifs_get/setxattr we can
> do that.

Yes, if other FSes have this check - please add it there.

> However the above implementation is consistent with ext2/ext3/ext4/jffs
> implementation.

OK, but on the other hand - how much sense does it make to have these
trivial wrappers? Should we have a wrapper per-check? :-)

BTW, to they have to be non-static?

> > Does an extended attribute in general with zero name length legitimate?
> My preference would be to remain consistent with interpretation of other
> file systems, in terms of what constitutes an
> invalid parameter. ext* filesystems seem to be declaring a blank
> extended attribute as invalid parameter. Man page for setxattr/getxattr
> don't explicitly state as such though.

Sure, let's add this check - I guess I was not careful enough and missed
it.

> > Did you check whether the generic code already performs this check?
> I didn't see a generic code that performs this check.

OK, thanks.

> >> + 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;
> > Where is the already allocated memory freed in this case?
> In this particular case kmalloc() failed and we are returning ENOMEM
> error, and in case of success, we do free the allocated memory.

Indeed, sorry for silly question.

> > You do not actually need these mutexes, because "inode" is new, it is
> > not added to any lists yet, so you own it entirely. Which means that you
> > do not even need to introduce this helper function - just call
> > 'security_inode_init_security()' directly.
> Okay, I can change the code to directly call the
> security_inode_init_security().

OK, thanks!

> It would great if someone else can run UBIFS with extended attributes
> enabled and provide an ACK! ;-)

I will run it once you send the patch I cannot nit-pick on anymore (aka
perfect patch) :-)))

Thanks!

--
Best Regards,
Artem Bityutskiy

Attachment: signature.asc
Description: This is a digitally signed message part