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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 15 May 2012 13:29:13 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Subodh Nijsure <snijsure@...d-net.com>
Cc:	linux-mtd@...ts.infradead.org, penguin-kernel@...ove.SAKURA.ne.jp,
	Adrian Hunter <adrian.hunter@...el.com>,
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v4] Add security.* XATTR support for the UBIFS

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@...d-net.com 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

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)