diff mbox

posix_acl: cleanup posix_acl_create()

Message ID 20150124193124.GA18322@mwanda (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter Jan. 24, 2015, 7:31 p.m. UTC 
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>

--
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

Comments

Omar Sandoval Jan. 27, 2015, 4:52 a.m. UTC | #1 
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
Dan Carpenter Jan. 27, 2015, 6:45 a.m. UTC | #2 
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

--
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
diff mbox

Patch

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);