Re: [PATCH v2 06/15] selinux: Use simple_start_creating() / simple_done_creating()

[Chris Mason <clm@meta.com>]

NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
> 
> Instead of explicitly locking the parent and performing a lookup in
> selinux, use simple_start_creating(), and then use
> simple_done_creating() to unlock.
> 

Hi everyone,

Amir suggested I run these through, and this commit was flagged:

commit bc981629767980bbaee2ca3c7348b5a648665a9d
Author: NeilBrown <neil@brown.name>

selinux: Use simple_start_creating() / simple_done_creating()

Instead of explicitly locking the parent and performing a lookup in
selinux, use simple_start_creating(), and then use
simple_done_creating() to unlock.

This extends the region that the directory is locked for, and also
performs a lookup. The lock extension is of no real consequence.
The lookup uses simple_lookup() and so always succeeds. Thus when
d_make_persistent() is called the dentry will already be hashed.
d_make_persistent() handles this case.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 3245cc531555b..7d4f90e5b12a0 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1931,15 +1931,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
>  static struct dentry *sel_make_swapover_dir(struct super_block *sb,
>  						unsigned long *ino)
>  {
> -	struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> +	struct dentry *dentry;
>  	struct inode *inode;
>
> -	if (!dentry)
> +	inode = sel_make_inode(sb, S_IFDIR);
> +	if (!inode)
>  		return ERR_PTR(-ENOMEM);
>
> -	inode = sel_make_inode(sb, S_IFDIR);
> -	if (!inode) {
> -		dput(dentry);
> +	dentry = simple_start_creating(sb->s_root, ".swapover");
> +	if (!dentry) {
> +		iput(inode);
>  		return ERR_PTR(-ENOMEM);
>  	}

Can this cause a kernel crash when simple_start_creating() fails?

The function simple_start_creating() returns ERR_PTR on failure, not
NULL. The check `if (!dentry)` only catches NULL values. When
simple_start_creating() fails and returns an ERR_PTR value like
ERR_PTR(-ENOMEM), this value is non-NULL and the error check is bypassed.

Execution would continue to d_make_persistent(dentry, inode), which
attempts to lock dentry->d_lock, dereferencing the ERR_PTR value and
causing a kernel crash.

The old code used d_alloc_name() which returns NULL on error. The new
code uses simple_start_creating() which returns ERR_PTR on error, but
the error checking pattern wasn't updated to match.

Should the check be `if (IS_ERR(dentry))` instead, and return dentry
rather than ERR_PTR(-ENOMEM) to preserve the actual error code?

>
> @@ -1947,11 +1948,9 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
>  	inode->i_ino = ++(*ino);
>  	/* directory inodes start off with i_nlink == 2 (for "." entry) */
>  	inc_nlink(inode);
> -	inode_lock(sb->s_root->d_inode);
>  	d_make_persistent(dentry, inode);
>  	inc_nlink(sb->s_root->d_inode);
> -	inode_unlock(sb->s_root->d_inode);
> -	dput(dentry);
> +	simple_done_creating(dentry);
>  	return dentry;	// borrowed
>  }