linux-kernel - Re: [PATCH v3] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <1334324512.10274.21.camel@moss-pluto>
Date:	Fri, 13 Apr 2012 09:41:52 -0400
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	subodh.nijsure@...il.com
Cc:	linux-mtd@...ts.infradead.org,
	Artem Bityutskiy <dedekind1@...il.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	Subodh Nijsure <snijsure@...d-net.com>
Subject: Re: [PATCH v3] Add security.* XATTR support for the UBIFS

On Wed, 2012-04-11 at 14:28 -0700, subodh.nijsure@...il.com wrote:
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index ec9f187..422bcec 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -292,6 +292,18 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
>  
>  	ubifs_release_budget(c, &req);
>  	insert_inode_hash(inode);
> +
> +#ifdef CONFIG_UBIFS_FS_XATTR
> +
> +	err = ubifs_init_security(dir, inode, &dentry->d_name);
> +	if (err) {
> +		ubifs_err("cannot initialize extended attribute, error %d",
> +			  err);
> +		goto out_cancel;
> +	}
> +
> +#endif
> +
>  	d_instantiate(dentry, inode);
>  	return 0;

I don't know this code very well, but simply reusing the out_cancel
error path when ubifs_init_security() fails looks wrong to me.  You'll
end up calling ubifs_release_budget() a second time.  You could move the
ubifs_init_security() call a bit earlier, prior to the
ubifs_release_budget() call, so that it only happens once on the error
path.  I also am unclear on what if anything you need to do about the
prior ubifs_jnl_update() if ubifs_init_security() fails; it seems like
you either need to update the journal again to reflect the inode
deletion or you need to move ubifs_init_security() prior to
ubifs_jnl_update() as well.

I don't know if this would work, but I would be inclined to move the
call to ubifs_init_security() immediately after the ubifs_new_inode()
call, and introduce a new out_security: label just before the
make_bad_inode() call to which you can jump on error.  However, that
might produce a strange state in the journal, with the setxattr recorded
before the inode creation.  Optimally they would both be part of the
same journal transaction, as in ext4, and thus applied atomically or not
at all (i.e. a file is either created with a security xattr or not at
all).

-- 
Stephen Smalley
National Security Agency

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/