Ocfs2/move_extents: fix error handling in ioctl

PATCH Superseded Dan Carpenter 3 participants 6 messages Apr 4, 2013 – Apr 9, 2013

Subsystems (2)

Kernel janitors OCFS2

lore

Share & tools

lore

Dan Carpenter

author

2013-04-04 06:37 UTC #0 lore raw

PATCH 1/2 Ocfs2/move_extents: fix error handling in ioctl

Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.

I've re-written the error handling to avoid this problem.

Signed-off-by Dan Carpenter <[email protected]>

fs/ocfs2/move_extents.c +11 ~~-10~~

index 9f8dcad..995d1b4 100644
@@ -1057,25 +1057,25 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 	struct inode *inode = file_inode(filp);
 	struct ocfs2_move_extents range;
-	struct ocfs2_move_extents_context *context = NULL;
+	struct ocfs2_move_extents_context *context;
 
 	status = mnt_want_write_file(filp);
 	if (status)
 		return status;
 
 	if ((!S_ISREG(inode->i_mode)) || !(filp->f_mode & FMODE_WRITE))
-		goto out;
+		goto out_drop;
 
 	if (inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
 		status = -EPERM;
-		goto out;
+		goto out_drop;
 	}
 
 	context = kzalloc(sizeof(struct ocfs2_move_extents_context), GFP_NOFS);
 	if (!context) {
 		status = -ENOMEM;
 		mlog_errno(status);
-		goto out;
+		goto out_drop;
 	}
 
 	context->inode = inode;

@@ -1084,15 +1084,15 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 	if (argp) {
 		if (copy_from_user(&range, argp, sizeof(range))) {
 			status = -EFAULT;
-			goto out;
+			goto out_free;
 		}
 	} else {
 		status = -EINVAL;
-		goto out;
+		goto out_free;
 	}
 
 	if (range.me_start > i_size_read(inode))
-		goto out;
+		goto out_free;
 
 	if (range.me_start + range.me_len > i_size_read(inode))
 			range.me_len = i_size_read(inode) - range.me_start;

@@ -1124,13 +1124,13 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 		status = ocfs2_validate_and_adjust_move_goal(inode, &range);
 		if (status)
-			goto out;
+			goto out_copy;
 	}
 
 	status = ocfs2_move_extents(context);
 	if (status)
 		mlog_errno(status);
-out:
+out_copy:
 	/*
 	 * movement/defragmentation may end up being partially completed,
 	 * that's the reason why we need to return userspace the finished

@@ -1141,8 +1141,9 @@ out:
 			status = -EFAULT;
 	}
 
+out_free:
 	kfree(context);
-
+out_drop:
 	mnt_drop_write_file(filp);
 
 	return status;

Jeff Liu

2013-04-04 09:21 UTC #1 lore raw

Hi Dan,

Quoted 3 lines · Dan Carpenter

Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.

Yes, We should copy the status of movement/defragmentation back to user only if this procedure has been issued although it may be only partially completed.

I personally would suggest to do a bit code adjustment for this function combine your current fix if you like, the related comments are inline.

Quoted 10 lines

I've re-written the error handling to avoid this problem.
Signed-off-by Dan Carpenter <[email protected]>
fs/ocfs2/move_extents.c +0 -0
index 9f8dcad..995d1b4 100644

fs/ocfs2/move_extents.c int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp) · line 1061

Quoted 6 lines

fs/ocfs2/move_extents.c +1 -1

@@ -1057,25 +1057,25 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 	struct inode *inode = file_inode(filp);
 	struct ocfs2_move_extents range;
-	struct ocfs2_move_extents_context *context = NULL;
+	struct ocfs2_move_extents_context *context;

Just return if argv is invalid, i.e. NULL.

	if (!argv)
		return -EINVAL;

Quoted 24 lines

fs/ocfs2/move_extents.c +3 -3

 	status = mnt_want_write_file(filp);
 	if (status)
 		return status;
 
 	if ((!S_ISREG(inode->i_mode)) || !(filp->f_mode & FMODE_WRITE))
-		goto out;
+		goto out_drop;
 
 	if (inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
 		status = -EPERM;
-		goto out;
+		goto out_drop;
 	}
 
 	context = kzalloc(sizeof(struct ocfs2_move_extents_context), GFP_NOFS);
 	if (!context) {
 		status = -ENOMEM;
 		mlog_errno(status);
-		goto out;
+		goto out_drop;
 	}
 
 	context->inode = inode;

fs/ocfs2/move_extents.c int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp) · line 1088

Quoted 6 lines

fs/ocfs2/move_extents.c +1 -1

@@ -1084,15 +1084,15 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 	if (argp) {
 		if (copy_from_user(&range, argp, sizeof(range))) {
 			status = -EFAULT;
-			goto out;
+			goto out_free;
 		}

With above pre-checking up against 'argv', we can kill the following conditional code block and try to copy the input range directly.

	if (copy_from_user(&range, ...)) {
		....
	}

Quoted 15 lines

fs/ocfs2/move_extents.c +2 -2

 	} else {
 		status = -EINVAL;
-		goto out;
+		goto out_free;
 	}
 
 	if (range.me_start > i_size_read(inode))
-		goto out;
+		goto out_free;
 
 	if (range.me_start + range.me_len > i_size_read(inode))
 			range.me_len = i_size_read(inode) - range.me_start;

@@ -1124,13 +1124,13 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 		status = ocfs2_validate_and_adjust_move_goal(inode, &range);

fs/ocfs2/move_extents.c int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp) · line 1133

Quoted 10 lines

fs/ocfs2/move_extents.c +2 -2

 		if (status)
-			goto out;
+			goto out_copy;
 	}
 
 	status = ocfs2_move_extents(context);
 	if (status)
 		mlog_errno(status);
-out:
+out_copy:

So with the beginning pre-checkup, we don't need to verify argp again that the original code does.
- if (argp) {
- if (copy_to_user(argp, &range, sizeof(range)))
- status = -EFAULT;
- }
+ if (copy_to_user(argp, &range, sizeof(range)))
+ status = -EFAULT;

Quoted 4 lines

fs/ocfs2/move_extents.c +0 -0

 	/*
 	 * movement/defragmentation may end up being partially completed,
 	 * that's the reason why we need to return userspace the finished

fs/ocfs2/move_extents.c out: · line 1149

Quoted 10 lines

fs/ocfs2/move_extents.c +2 -1

@@ -1141,8 +1141,9 @@ out:
 			status = -EFAULT;
 	}
 
+out_free:
 	kfree(context);
-
+out_drop:
 	mnt_drop_write_file(filp);
 
 	return status;

Thanks,
-Jeff

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2013-04-04 11:40 UTC #2 lore raw

PATCH v2 1/2 Ocfs2/move_extents: fix error handling in ioctl

Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.

I've re-written the error handling to avoid this problem and make it a little cleaner as well.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: check for argp earlier

fs/ocfs2/move_extents.c +17 ~~-20~~

index 9f8dcad..8f3d3cb 100644
@@ -1057,42 +1057,40 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 	struct inode *inode = file_inode(filp);
 	struct ocfs2_move_extents range;
-	struct ocfs2_move_extents_context *context = NULL;
+	struct ocfs2_move_extents_context *context;
+
+	if (!argp)
+		return -EINVAL;
 
 	status = mnt_want_write_file(filp);
 	if (status)
 		return status;
 
 	if ((!S_ISREG(inode->i_mode)) || !(filp->f_mode & FMODE_WRITE))
-		goto out;
+		goto out_drop;
 
 	if (inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
 		status = -EPERM;
-		goto out;
+		goto out_drop;
 	}
 
 	context = kzalloc(sizeof(struct ocfs2_move_extents_context), GFP_NOFS);
 	if (!context) {
 		status = -ENOMEM;
 		mlog_errno(status);
-		goto out;
+		goto out_drop;
 	}
 
 	context->inode = inode;
 	context->file = filp;
 
-	if (argp) {
-		if (copy_from_user(&range, argp, sizeof(range))) {
-			status = -EFAULT;
-			goto out;
-		}
-	} else {
-		status = -EINVAL;
-		goto out;
+	if (copy_from_user(&range, argp, sizeof(range))) {
+		status = -EFAULT;
+		goto out_free;
 	}
 
 	if (range.me_start > i_size_read(inode))
-		goto out;
+		goto out_free;
 
 	if (range.me_start + range.me_len > i_size_read(inode))
 			range.me_len = i_size_read(inode) - range.me_start;

@@ -1124,25 +1122,24 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 		status = ocfs2_validate_and_adjust_move_goal(inode, &range);
 		if (status)
-			goto out;
+			goto out_copy;
 	}
 
 	status = ocfs2_move_extents(context);
 	if (status)
 		mlog_errno(status);
-out:
+out_copy:
 	/*
 	 * movement/defragmentation may end up being partially completed,
 	 * that's the reason why we need to return userspace the finished
 	 * length and new_offset even if failure happens somewhere.
 	 */
-	if (argp) {
-		if (copy_to_user(argp, &range, sizeof(range)))
-			status = -EFAULT;
-	}
+	if (copy_to_user(argp, &range, sizeof(range)))
+		status = -EFAULT;
 
+out_free:
 	kfree(context);
-
+out_drop:
 	mnt_drop_write_file(filp);
 
 	return status;

Jeff Liu

Reviewed-by on v2 1/2

2013-04-04 11:52 UTC #3 lore raw

Quoted 8 lines · Dan Carpenter

Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.
I've re-written the error handling to avoid this problem and make it a little cleaner as well.
Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Jie Liu <[email protected]>

Quoted 98 lines

---
v2: check for argp earlier

fs/ocfs2/move_extents.c +17 ~~-21~~

index 9f8dcad..8f3d3cb 100644
@@ -1057,42 +1057,40 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 	struct inode *inode = file_inode(filp);
 	struct ocfs2_move_extents range;
-	struct ocfs2_move_extents_context *context = NULL;
+	struct ocfs2_move_extents_context *context;
+
+	if (!argp)
+		return -EINVAL;
 
 	status = mnt_want_write_file(filp);
 	if (status)
 		return status;
 
 	if ((!S_ISREG(inode->i_mode)) || !(filp->f_mode & FMODE_WRITE))
-		goto out;
+		goto out_drop;
 
 	if (inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
 		status = -EPERM;
-		goto out;
+		goto out_drop;
 	}
 
 	context = kzalloc(sizeof(struct ocfs2_move_extents_context), GFP_NOFS);
 	if (!context) {
 		status = -ENOMEM;
 		mlog_errno(status);
-		goto out;
+		goto out_drop;
 	}
 
 	context->inode = inode;
 	context->file = filp;
 
-	if (argp) {
-		if (copy_from_user(&range, argp, sizeof(range))) {
-			status = -EFAULT;
-			goto out;
-		}
-	} else {
-		status = -EINVAL;
-		goto out;
+	if (copy_from_user(&range, argp, sizeof(range))) {
+		status = -EFAULT;
+		goto out_free;
 	}
 
 	if (range.me_start > i_size_read(inode))
-		goto out;
+		goto out_free;
 
 	if (range.me_start + range.me_len > i_size_read(inode))
 			range.me_len = i_size_read(inode) - range.me_start;

@@ -1124,25 +1122,24 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 		status = ocfs2_validate_and_adjust_move_goal(inode, &range);
 		if (status)
-			goto out;
+			goto out_copy;
 	}
 
 	status = ocfs2_move_extents(context);
 	if (status)
 		mlog_errno(status);
-out:
+out_copy:
 	/*
 	 * movement/defragmentation may end up being partially completed,
 	 * that's the reason why we need to return userspace the finished
 	 * length and new_offset even if failure happens somewhere.
 	 */
-	if (argp) {
-		if (copy_to_user(argp, &range, sizeof(range)))
-			status = -EFAULT;
-	}
+	if (copy_to_user(argp, &range, sizeof(range)))
+		status = -EFAULT;
 
+out_free:
 	kfree(context);
-
+out_drop:
 	mnt_drop_write_file(filp);
 
 	return status;
--

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andrew Morton

2013-04-09 00:04 UTC #4 lore raw

Quoted 10 lines · On Thu, 04 Apr 2013 19:52:36 +0800 Jeff Liu <[email protected]> wrote:

Quoted 8 lines · Dan Carpenter
Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.
I've re-written the error handling to avoid this problem and make it a little cleaner as well.
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Jie Liu <[email protected]>

I can't find either of Dan's emails anywhere. Confused.

Resend both, please?

Jeff Liu

2013-04-09 03:13 UTC #5 lore raw

Quoted 10 lines · Andrew Morton

Quoted 8 lines · On Thu, 04 Apr 2013 19:52:36 +0800 Jeff Liu <[email protected]> wrote:
Quoted 7 lines · Dan Carpenter
Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.
I've re-written the error handling to avoid this problem and make it a little cleaner as well.

Quoted 6 lines

Quoted 2 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Jie Liu <[email protected]>
I can't find either of Dan's emails anywhere. Confused.
Resend both, please?

Looks they were eaten by the mailing list somehow, I'll resent both of them on behalf of Dan in a minute.

Thanks,
-Jeff

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html