Skip to contentSkip to conversation

posix_acl: cleanup posix_acl_create()

PATCH In review Dan Carpenter 3 participants 6 messages Jan 24, 2015 – Mar 9, 2015

Subsystems (3)
VFS Kernel janitors OCFS2
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 posix_acl: cleanup posix_acl_create()

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 <[email protected]>
fs/posix_acl.c +20 -27
index 0855f77..66d2c13 100644
@@ -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);
#1 lore raw

Hi, Dan,

Quoted 5 lines · 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.

Quoted 7 lines
	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.

Quoted 81 lines
Signed-off-by Dan Carpenter <[email protected]>
fs/posix_acl.c +20 -28
index 0855f77..66d2c13 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line
Omar
author
#2 lore raw
Quoted 15 lines · Omar Sandoval

Hi, Dan,

Quoted 5 lines · 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.

Quoted 7 lines
	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.

Quoted 6 lines

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

author
#3 lore raw

PATCH 1/2 ocfs2: dereferencing freed pointers in ocfs2_reflink()

In reply to 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 <[email protected]>
fs/ocfs2/refcounttree.c +1 -1
index ee541f9..df3a500 100644
@@ -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,
author
#5 lore raw

PATCH v2 2/2 posix_acl: make posix_acl_create() safer and cleaner

In reply to 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 <[email protected]>

---
v2: Update to latest kernel.

fs/posix_acl.c +20 -26
index 3a48bb7..a327300 100644
@@ -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;