[patch] posix_acl: cleanup posix_acl_create()

[patch] posix_acl: cleanup posix_acl_create()

[Dan Carpenter]

If posix_acl_create() returns an error code then "*acl" and
"*default_acl" can be uninitialized or point to freed memory.  This
causes problems in some of the callers where it is expected that they
are NULL on error.  For example, ocfs2_reflink() has a bug.

	fs/ocfs2/refcounttree.c:4329 ocfs2_reflink()
	error: potentially using uninitialized 'default_acl'.

I have re-written this function and re-arranged things so that they are
set to NULL at the start and then only set to a valid pointer at the end
of the function.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 0855f77..66d2c13 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -546,50 +546,43 @@ int
 posix_acl_create(struct inode *dir, umode_t *mode,
 		struct posix_acl **default_acl, struct posix_acl **acl)
 {
-	struct posix_acl *p;
+	struct posix_acl *p, *clone;
 	int ret;
 
+	*acl = NULL;
+	*default_acl = NULL;
+
 	if (S_ISLNK(*mode) || !IS_POSIXACL(dir))
-		goto no_acl;
+		return 0;
 
 	p = get_acl(dir, ACL_TYPE_DEFAULT);
-	if (IS_ERR(p)) {
-		if (p == ERR_PTR(-EOPNOTSUPP))
-			goto apply_umask;
-		return PTR_ERR(p);
+	if (!p || p == ERR_PTR(-EOPNOTSUPP)) {
+		*mode &= ~current_umask();
+		return 0;
 	}
+	if (IS_ERR(p))
+		return PTR_ERR(p);
 
-	if (!p)
-		goto apply_umask;
-
-	*acl = posix_acl_clone(p, GFP_NOFS);
-	if (!*acl)
+	clone = posix_acl_clone(p, GFP_NOFS);
+	if (!clone)
 		return -ENOMEM;
 
-	ret = posix_acl_create_masq(*acl, mode);
+	ret = posix_acl_create_masq(clone, mode);
 	if (ret < 0) {
-		posix_acl_release(*acl);
+		posix_acl_release(clone);
 		return -ENOMEM;
 	}
 
-	if (ret == 0) {
-		posix_acl_release(*acl);
-		*acl = NULL;
-	}
+	if (ret == 0)
+		posix_acl_release(clone);
+	else
+		*acl = clone;
 
-	if (!S_ISDIR(*mode)) {
+	if (!S_ISDIR(*mode))
 		posix_acl_release(p);
-		*default_acl = NULL;
-	} else {
+	else
 		*default_acl = p;
-	}
-	return 0;
 
-apply_umask:
-	*mode &= ~current_umask();
-no_acl:
-	*default_acl = NULL;
-	*acl = NULL;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(posix_acl_create);

Re: [patch] posix_acl: cleanup posix_acl_create()

[Omar Sandoval]

Hi, Dan,

On Sat, Jan 24, 2015 at 10:31:24PM +0300, Dan Carpenter wrote:
> If posix_acl_create() returns an error code then "*acl" and
> "*default_acl" can be uninitialized or point to freed memory.  This
> causes problems in some of the callers where it is expected that they
> are NULL on error.  For example, ocfs2_reflink() has a bug.
> 
> 	fs/ocfs2/refcounttree.c:4329 ocfs2_reflink()
> 	error: potentially using uninitialized 'default_acl'.
> 
> I have re-written this function and re-arranged things so that they are
> set to NULL at the start and then only set to a valid pointer at the end
> of the function.
> 
I'm inclined to blame ocfs2 and not posix_acl_create() here. I'd imagine
that most C programmers' intuition is generally not to trust any return
parameters when the return value is an error. Accordingly, a quick scan
of the tree showed that all of the other users of posix_acl_create() are
doing it correctly and only calling posix_acl_release() when it returns
success.

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 0855f77..66d2c13 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -546,50 +546,43 @@ int
>  posix_acl_create(struct inode *dir, umode_t *mode,
>  		struct posix_acl **default_acl, struct posix_acl **acl)
>  {
> -	struct posix_acl *p;
> +	struct posix_acl *p, *clone;
>  	int ret;
>  
> +	*acl = NULL;
> +	*default_acl = NULL;
> +
>  	if (S_ISLNK(*mode) || !IS_POSIXACL(dir))
> -		goto no_acl;
> +		return 0;
>  
>  	p = get_acl(dir, ACL_TYPE_DEFAULT);
> -	if (IS_ERR(p)) {
> -		if (p == ERR_PTR(-EOPNOTSUPP))
> -			goto apply_umask;
> -		return PTR_ERR(p);
> +	if (!p || p == ERR_PTR(-EOPNOTSUPP)) {
> +		*mode &= ~current_umask();
> +		return 0;
>  	}
> +	if (IS_ERR(p))
> +		return PTR_ERR(p);
>  
> -	if (!p)
> -		goto apply_umask;
> -
> -	*acl = posix_acl_clone(p, GFP_NOFS);
> -	if (!*acl)
> +	clone = posix_acl_clone(p, GFP_NOFS);
> +	if (!clone)
>  		return -ENOMEM;
>  
> -	ret = posix_acl_create_masq(*acl, mode);
> +	ret = posix_acl_create_masq(clone, mode);
>  	if (ret < 0) {
> -		posix_acl_release(*acl);
> +		posix_acl_release(clone);
>  		return -ENOMEM;
>  	}
>  
> -	if (ret == 0) {
> -		posix_acl_release(*acl);
> -		*acl = NULL;
> -	}
> +	if (ret == 0)
> +		posix_acl_release(clone);
> +	else
> +		*acl = clone;
>  
> -	if (!S_ISDIR(*mode)) {
> +	if (!S_ISDIR(*mode))
>  		posix_acl_release(p);
> -		*default_acl = NULL;
> -	} else {
> +	else
>  		*default_acl = p;
> -	}
> -	return 0;
>  
> -apply_umask:
> -	*mode &= ~current_umask();
> -no_acl:
> -	*default_acl = NULL;
> -	*acl = NULL;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(posix_acl_create);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Omar

Re: [patch] posix_acl: cleanup posix_acl_create()

[Dan Carpenter]

On Mon, Jan 26, 2015 at 08:52:09PM -0800, Omar Sandoval wrote:
> Hi, Dan,
> 
> On Sat, Jan 24, 2015 at 10:31:24PM +0300, Dan Carpenter wrote:
> > If posix_acl_create() returns an error code then "*acl" and
> > "*default_acl" can be uninitialized or point to freed memory.  This
> > causes problems in some of the callers where it is expected that they
> > are NULL on error.  For example, ocfs2_reflink() has a bug.
> > 
> > 	fs/ocfs2/refcounttree.c:4329 ocfs2_reflink()
> > 	error: potentially using uninitialized 'default_acl'.
> > 
> > I have re-written this function and re-arranged things so that they are
> > set to NULL at the start and then only set to a valid pointer at the end
> > of the function.
> > 
> I'm inclined to blame ocfs2 and not posix_acl_create() here. I'd imagine
> that most C programmers' intuition is generally not to trust any return
> parameters when the return value is an error. Accordingly, a quick scan
> of the tree showed that all of the other users of posix_acl_create() are
> doing it correctly and only calling posix_acl_release() when it returns
> success.

Both ocfs2_reflink() and posix_acl_create() are ugly.

ocfs2_reflink() uses an "out:" label.  Whenever you see an "out:" label
then, hopefully, people are just too lazy to name their labels but a lot
of the time an out label means the code is going to do something stupid.
In this case, it's doing "one err" style error handling where it just
has one error label that tries to free everything including the stuff
that was never allocated.  One Err Bugs like this are very common in the
kernel.

Still it's bad to return freed pointers because you know that it's going
to cause double frees.  And also posix_acl_create() was messy in
general.

Both functions should be cleaned up but I am too busy to clean up the
whole kernel.

regards,
dan carpenter

[patch 1/2] ocfs2: dereferencing freed pointers in ocfs2_reflink()

[Dan Carpenter]

The code at the "out" label assumes that "default_acl" and "acl" are
NULL, but actually the pointers can be NULL, unitialized, or freed.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index ee541f9..df3a500 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4276,7 +4276,7 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
 	error = posix_acl_create(dir, &mode, &default_acl, &acl);
 	if (error) {
 		mlog_errno(error);
-		goto out;
+		return error;
 	}
 
 	error = ocfs2_create_inode_in_orphan(dir, mode,

[patch 2/2 v2] posix_acl: make posix_acl_create() safer and cleaner

[Dan Carpenter]

If posix_acl_create() returns an error code then "*acl" and
"*default_acl" can be uninitialized or point to freed memory.  This is a
dangerous thing to do.  For example, it causes a problem in
ocfs2_reflink():

	fs/ocfs2/refcounttree.c:4327 ocfs2_reflink()
	error: potentially using uninitialized 'default_acl'.

I've re-written this so we set the pointers to NULL at the start.
I've added a temporary "clone" variable to hold the value of "*acl"
until end.  Setting them to NULL means means we don't need the "no_acl"
label.  We may as well remove the "apply_umask" stuff forward and remove
that label as well.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Update to latest kernel.

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 3a48bb7..a327300 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -547,51 +547,45 @@ posix_acl_create(struct inode *dir, umode_t *mode,
 		struct posix_acl **default_acl, struct posix_acl **acl)
 {
 	struct posix_acl *p;
+	struct posix_acl *clone;
 	int ret;
 
+	*acl = NULL;
+	*default_acl = NULL;
+
 	if (S_ISLNK(*mode) || !IS_POSIXACL(dir))
-		goto no_acl;
+		return 0;
 
 	p = get_acl(dir, ACL_TYPE_DEFAULT);
-	if (IS_ERR(p)) {
-		if (p == ERR_PTR(-EOPNOTSUPP))
-			goto apply_umask;
-		return PTR_ERR(p);
+	if (!p || p == ERR_PTR(-EOPNOTSUPP)) {
+		*mode &= ~current_umask();
+		return 0;
 	}
+	if (IS_ERR(p))
+		return PTR_ERR(p);
 
-	if (!p)
-		goto apply_umask;
-
-	*acl = posix_acl_clone(p, GFP_NOFS);
-	if (!*acl)
+	clone = posix_acl_clone(p, GFP_NOFS);
+	if (!clone)
 		goto no_mem;
 
-	ret = posix_acl_create_masq(*acl, mode);
+	ret = posix_acl_create_masq(clone, mode);
 	if (ret < 0)
 		goto no_mem_clone;
 
-	if (ret == 0) {
-		posix_acl_release(*acl);
-		*acl = NULL;
-	}
+	if (ret == 0)
+		posix_acl_release(clone);
+	else
+		*acl = clone;
 
-	if (!S_ISDIR(*mode)) {
+	if (!S_ISDIR(*mode))
 		posix_acl_release(p);
-		*default_acl = NULL;
-	} else {
+	else
 		*default_acl = p;
-	}
-	return 0;
 
-apply_umask:
-	*mode &= ~current_umask();
-no_acl:
-	*default_acl = NULL;
-	*acl = NULL;
 	return 0;
 
 no_mem_clone:
-	posix_acl_release(*acl);
+	posix_acl_release(clone);
 no_mem:
 	posix_acl_release(p);
 	return -ENOMEM;

Re: [patch 1/2] ocfs2: dereferencing freed pointers in ocfs2_reflink()

[Mark Fasheh]

On Thu, Mar 05, 2015 at 08:46:19PM +0300, Dan Carpenter wrote:
> The code at the "out" label assumes that "default_acl" and "acl" are
> NULL, but actually the pointers can be NULL, unitialized, or freed.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Mark Fasheh <mfasheh@suse.de>

--
Mark Fasheh