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:	Mon, 14 May 2012 16:02:18 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	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 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 ?
Does an extended attribute in general with zero name length legitimate?
Did you check whether the generic code already performs this check?

> +	return __ubifs_setxattr(d->d_inode, 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;

Where is the already allocated memory freed in this case?

> +		}
> +		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)
> +{
> +	struct ubifs_inode *dir_ui = ubifs_inode(inode);
> +	int err = 0;
> +
> +	mutex_lock(&inode->i_mutex);
> +	mutex_lock(&dir_ui->ui_mutex);
> +

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.


-- 
Best Regards,
Artem Bityutskiy

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