All of lore.kernel.org
 help / color / mirror / Atom feed
* [Ocfs2-devel] [PATCH] ocfs2: Do not write error flag to user structure we cannot copy from/to
@ 2013-10-27 20:18 Ben Hutchings
  2013-11-07 11:22 ` Joel Becker
  0 siblings, 1 reply; 3+ messages in thread
From: Ben Hutchings @ 2013-10-27 20:18 UTC (permalink / raw)
  To: ocfs2-devel

If we failed to copy from the structure, writing back the flags leaks
31 bits of kernel memory (the rest of the ir_flags field).

In any case, if we cannot copy from/to the structure, why should we
expect putting just the flags to work?

Also make sure ocfs2_info_handle_freeinode() returns the right error
code if the copy_to_user() fails.

Compile-tested only.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: stable at vger.kernel.org
Fixes: ddee5cdb70e6 ('Ocfs2: Add new OCFS2_IOC_INFO ioctl for ocfs2 v8.')
---
 fs/ocfs2/ioctl.c | 129 +++++++++++++++++++------------------------------------
 1 file changed, 43 insertions(+), 86 deletions(-)

diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
index fa32ce9..71e2492 100644
--- a/fs/ocfs2/ioctl.c
+++ b/fs/ocfs2/ioctl.c
@@ -34,9 +34,8 @@
 		copy_to_user((typeof(a) __user *)b, &(a), sizeof(a))
 
 /*
- * This call is void because we are already reporting an error that may
- * be -EFAULT.  The error will be returned from the ioctl(2) call.  It's
- * just a best-effort to tell userspace that this request caused the error.
+ * This is just a best-effort to tell userspace that this request
+ * caused the error.
  */
 static inline void o2info_set_request_error(struct ocfs2_info_request *kreq,
 					struct ocfs2_info_request __user *req)
@@ -145,136 +144,105 @@ bail:
 int ocfs2_info_handle_blocksize(struct inode *inode,
 				struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_blocksize oib;
 
 	if (o2info_from_user(oib, req))
-		goto bail;
+		return -EFAULT;
 
 	oib.ib_blocksize = inode->i_sb->s_blocksize;
 
 	o2info_set_request_filled(&oib.ib_req);
 
 	if (o2info_to_user(oib, req))
-		goto bail;
-
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oib.ib_req, req);
+		return -EFAULT;
 
-	return status;
+	return 0;
 }
 
 int ocfs2_info_handle_clustersize(struct inode *inode,
 				  struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_clustersize oic;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 
 	if (o2info_from_user(oic, req))
-		goto bail;
+		return -EFAULT;
 
 	oic.ic_clustersize = osb->s_clustersize;
 
 	o2info_set_request_filled(&oic.ic_req);
 
 	if (o2info_to_user(oic, req))
-		goto bail;
-
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oic.ic_req, req);
+		return -EFAULT;
 
-	return status;
+	return 0;
 }
 
 int ocfs2_info_handle_maxslots(struct inode *inode,
 			       struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_maxslots oim;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 
 	if (o2info_from_user(oim, req))
-		goto bail;
+		return -EFAULT;
 
 	oim.im_max_slots = osb->max_slots;
 
 	o2info_set_request_filled(&oim.im_req);
 
 	if (o2info_to_user(oim, req))
-		goto bail;
+		return -EFAULT;
 
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oim.im_req, req);
-
-	return status;
+	return 0;
 }
 
 int ocfs2_info_handle_label(struct inode *inode,
 			    struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_label oil;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 
 	if (o2info_from_user(oil, req))
-		goto bail;
+		return -EFAULT;
 
 	memcpy(oil.il_label, osb->vol_label, OCFS2_MAX_VOL_LABEL_LEN);
 
 	o2info_set_request_filled(&oil.il_req);
 
 	if (o2info_to_user(oil, req))
-		goto bail;
+		return -EFAULT;
 
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oil.il_req, req);
-
-	return status;
+	return 0;
 }
 
 int ocfs2_info_handle_uuid(struct inode *inode,
 			   struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_uuid oiu;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 
 	if (o2info_from_user(oiu, req))
-		goto bail;
+		return -EFAULT;
 
 	memcpy(oiu.iu_uuid_str, osb->uuid_str, OCFS2_TEXT_UUID_LEN + 1);
 
 	o2info_set_request_filled(&oiu.iu_req);
 
 	if (o2info_to_user(oiu, req))
-		goto bail;
-
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oiu.iu_req, req);
+		return -EFAULT;
 
-	return status;
+	return 0;
 }
 
 int ocfs2_info_handle_fs_features(struct inode *inode,
 				  struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_fs_features oif;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 
 	if (o2info_from_user(oif, req))
-		goto bail;
+		return -EFAULT;
 
 	oif.if_compat_features = osb->s_feature_compat;
 	oif.if_incompat_features = osb->s_feature_incompat;
@@ -283,39 +251,28 @@ int ocfs2_info_handle_fs_features(struct inode *inode,
 	o2info_set_request_filled(&oif.if_req);
 
 	if (o2info_to_user(oif, req))
-		goto bail;
+		return -EFAULT;
 
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oif.if_req, req);
-
-	return status;
+	return 0;
 }
 
 int ocfs2_info_handle_journal_size(struct inode *inode,
 				   struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_journal_size oij;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 
 	if (o2info_from_user(oij, req))
-		goto bail;
+		return -EFAULT;
 
 	oij.ij_journal_size = i_size_read(osb->journal->j_inode);
 
 	o2info_set_request_filled(&oij.ij_req);
 
 	if (o2info_to_user(oij, req))
-		goto bail;
+		return -EFAULT;
 
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oij.ij_req, req);
-
-	return status;
+	return 0;
 }
 
 int ocfs2_info_scan_inode_alloc(struct ocfs2_super *osb,
@@ -371,7 +328,7 @@ int ocfs2_info_handle_freeinode(struct inode *inode,
 	u32 i;
 	u64 blkno = -1;
 	char namebuf[40];
-	int status = -EFAULT, type = INODE_ALLOC_SYSTEM_INODE;
+	int status, type = INODE_ALLOC_SYSTEM_INODE;
 	struct ocfs2_info_freeinode *oifi = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct inode *inode_alloc = NULL;
@@ -383,8 +340,10 @@ int ocfs2_info_handle_freeinode(struct inode *inode,
 		goto out_err;
 	}
 
-	if (o2info_from_user(*oifi, req))
-		goto bail;
+	if (o2info_from_user(*oifi, req)) {
+		status = -EFAULT;
+		goto out_free;
+	}
 
 	oifi->ifi_slotnum = osb->max_slots;
 
@@ -421,14 +380,16 @@ int ocfs2_info_handle_freeinode(struct inode *inode,
 
 	o2info_set_request_filled(&oifi->ifi_req);
 
-	if (o2info_to_user(*oifi, req))
-		goto bail;
+	if (o2info_to_user(*oifi, req)) {
+		status = -EFAULT;
+		goto out_free;
+	}
 
 	status = 0;
 bail:
 	if (status)
 		o2info_set_request_error(&oifi->ifi_req, req);
-
+out_free:
 	kfree(oifi);
 out_err:
 	return status;
@@ -655,7 +616,7 @@ int ocfs2_info_handle_freefrag(struct inode *inode,
 {
 	u64 blkno = -1;
 	char namebuf[40];
-	int status = -EFAULT, type = GLOBAL_BITMAP_SYSTEM_INODE;
+	int status, type = GLOBAL_BITMAP_SYSTEM_INODE;
 
 	struct ocfs2_info_freefrag *oiff;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
@@ -668,8 +629,10 @@ int ocfs2_info_handle_freefrag(struct inode *inode,
 		goto out_err;
 	}
 
-	if (o2info_from_user(*oiff, req))
-		goto bail;
+	if (o2info_from_user(*oiff, req)) {
+		status = -EFAULT;
+		goto out_free;
+	}
 	/*
 	 * chunksize from userspace should be power of 2.
 	 */
@@ -708,14 +671,14 @@ int ocfs2_info_handle_freefrag(struct inode *inode,
 
 	if (o2info_to_user(*oiff, req)) {
 		status = -EFAULT;
-		goto bail;
+		goto out_free;
 	}
 
 	status = 0;
 bail:
 	if (status)
 		o2info_set_request_error(&oiff->iff_req, req);
-
+out_free:
 	kfree(oiff);
 out_err:
 	return status;
@@ -724,23 +687,17 @@ out_err:
 int ocfs2_info_handle_unknown(struct inode *inode,
 			      struct ocfs2_info_request __user *req)
 {
-	int status = -EFAULT;
 	struct ocfs2_info_request oir;
 
 	if (o2info_from_user(oir, req))
-		goto bail;
+		return -EFAULT;
 
 	o2info_clear_request_filled(&oir);
 
 	if (o2info_to_user(oir, req))
-		goto bail;
+		return -EFAULT;
 
-	status = 0;
-bail:
-	if (status)
-		o2info_set_request_error(&oir, req);
-
-	return status;
+	return 0;
 }
 
 /*

-- 
Ben Hutchings
If at first you don't succeed, you're doing about average.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 828 bytes
Desc: This is a digitally signed message part
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20131027/35babc40/attachment-0001.bin 

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* [Ocfs2-devel] [PATCH] ocfs2: Do not write error flag to user structure we cannot copy from/to
  2013-10-27 20:18 [Ocfs2-devel] [PATCH] ocfs2: Do not write error flag to user structure we cannot copy from/to Ben Hutchings
@ 2013-11-07 11:22 ` Joel Becker
  2013-11-08  1:21   ` Ben Hutchings
  0 siblings, 1 reply; 3+ messages in thread
From: Joel Becker @ 2013-11-07 11:22 UTC (permalink / raw)
  To: ocfs2-devel

On Sun, Oct 27, 2013 at 08:18:02PM +0000, Ben Hutchings wrote:
> If we failed to copy from the structure, writing back the flags leaks
> 31 bits of kernel memory (the rest of the ir_flags field).
> 
> In any case, if we cannot copy from/to the structure, why should we
> expect putting just the flags to work?

The first issue could be fixed; we could clear the flags.  The second
issue is what matters.  If we're getting EFAULT, we're not going to be
moving any flags around.  We should just return the EFAULT and be done
with it.
 
> Also make sure ocfs2_info_handle_freeinode() returns the right error
> code if the copy_to_user() fails.
> 
> Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: stable at vger.kernel.org
> Fixes: ddee5cdb70e6 ('Ocfs2: Add new OCFS2_IOC_INFO ioctl for ocfs2 v8.')

I can't recommend this for stable if it hasn't actually been tested.
There is no exposure; as you point out, the put_user() will fail if the
get_user() is failing.

Send me a version without stable and I'll Ack it.

Joel


> ---
>  fs/ocfs2/ioctl.c | 129 +++++++++++++++++++------------------------------------
>  1 file changed, 43 insertions(+), 86 deletions(-)
> 
> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
> index fa32ce9..71e2492 100644
> --- a/fs/ocfs2/ioctl.c
> +++ b/fs/ocfs2/ioctl.c
> @@ -34,9 +34,8 @@
>  		copy_to_user((typeof(a) __user *)b, &(a), sizeof(a))
>  
>  /*
> - * This call is void because we are already reporting an error that may
> - * be -EFAULT.  The error will be returned from the ioctl(2) call.  It's
> - * just a best-effort to tell userspace that this request caused the error.
> + * This is just a best-effort to tell userspace that this request
> + * caused the error.
>   */
>  static inline void o2info_set_request_error(struct ocfs2_info_request *kreq,
>  					struct ocfs2_info_request __user *req)
> @@ -145,136 +144,105 @@ bail:
>  int ocfs2_info_handle_blocksize(struct inode *inode,
>  				struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_blocksize oib;
>  
>  	if (o2info_from_user(oib, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	oib.ib_blocksize = inode->i_sb->s_blocksize;
>  
>  	o2info_set_request_filled(&oib.ib_req);
>  
>  	if (o2info_to_user(oib, req))
> -		goto bail;
> -
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oib.ib_req, req);
> +		return -EFAULT;
>  
> -	return status;
> +	return 0;
>  }
>  
>  int ocfs2_info_handle_clustersize(struct inode *inode,
>  				  struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_clustersize oic;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  
>  	if (o2info_from_user(oic, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	oic.ic_clustersize = osb->s_clustersize;
>  
>  	o2info_set_request_filled(&oic.ic_req);
>  
>  	if (o2info_to_user(oic, req))
> -		goto bail;
> -
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oic.ic_req, req);
> +		return -EFAULT;
>  
> -	return status;
> +	return 0;
>  }
>  
>  int ocfs2_info_handle_maxslots(struct inode *inode,
>  			       struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_maxslots oim;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  
>  	if (o2info_from_user(oim, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	oim.im_max_slots = osb->max_slots;
>  
>  	o2info_set_request_filled(&oim.im_req);
>  
>  	if (o2info_to_user(oim, req))
> -		goto bail;
> +		return -EFAULT;
>  
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oim.im_req, req);
> -
> -	return status;
> +	return 0;
>  }
>  
>  int ocfs2_info_handle_label(struct inode *inode,
>  			    struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_label oil;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  
>  	if (o2info_from_user(oil, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	memcpy(oil.il_label, osb->vol_label, OCFS2_MAX_VOL_LABEL_LEN);
>  
>  	o2info_set_request_filled(&oil.il_req);
>  
>  	if (o2info_to_user(oil, req))
> -		goto bail;
> +		return -EFAULT;
>  
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oil.il_req, req);
> -
> -	return status;
> +	return 0;
>  }
>  
>  int ocfs2_info_handle_uuid(struct inode *inode,
>  			   struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_uuid oiu;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  
>  	if (o2info_from_user(oiu, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	memcpy(oiu.iu_uuid_str, osb->uuid_str, OCFS2_TEXT_UUID_LEN + 1);
>  
>  	o2info_set_request_filled(&oiu.iu_req);
>  
>  	if (o2info_to_user(oiu, req))
> -		goto bail;
> -
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oiu.iu_req, req);
> +		return -EFAULT;
>  
> -	return status;
> +	return 0;
>  }
>  
>  int ocfs2_info_handle_fs_features(struct inode *inode,
>  				  struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_fs_features oif;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  
>  	if (o2info_from_user(oif, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	oif.if_compat_features = osb->s_feature_compat;
>  	oif.if_incompat_features = osb->s_feature_incompat;
> @@ -283,39 +251,28 @@ int ocfs2_info_handle_fs_features(struct inode *inode,
>  	o2info_set_request_filled(&oif.if_req);
>  
>  	if (o2info_to_user(oif, req))
> -		goto bail;
> +		return -EFAULT;
>  
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oif.if_req, req);
> -
> -	return status;
> +	return 0;
>  }
>  
>  int ocfs2_info_handle_journal_size(struct inode *inode,
>  				   struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_journal_size oij;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  
>  	if (o2info_from_user(oij, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	oij.ij_journal_size = i_size_read(osb->journal->j_inode);
>  
>  	o2info_set_request_filled(&oij.ij_req);
>  
>  	if (o2info_to_user(oij, req))
> -		goto bail;
> +		return -EFAULT;
>  
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oij.ij_req, req);
> -
> -	return status;
> +	return 0;
>  }
>  
>  int ocfs2_info_scan_inode_alloc(struct ocfs2_super *osb,
> @@ -371,7 +328,7 @@ int ocfs2_info_handle_freeinode(struct inode *inode,
>  	u32 i;
>  	u64 blkno = -1;
>  	char namebuf[40];
> -	int status = -EFAULT, type = INODE_ALLOC_SYSTEM_INODE;
> +	int status, type = INODE_ALLOC_SYSTEM_INODE;
>  	struct ocfs2_info_freeinode *oifi = NULL;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  	struct inode *inode_alloc = NULL;
> @@ -383,8 +340,10 @@ int ocfs2_info_handle_freeinode(struct inode *inode,
>  		goto out_err;
>  	}
>  
> -	if (o2info_from_user(*oifi, req))
> -		goto bail;
> +	if (o2info_from_user(*oifi, req)) {
> +		status = -EFAULT;
> +		goto out_free;
> +	}
>  
>  	oifi->ifi_slotnum = osb->max_slots;
>  
> @@ -421,14 +380,16 @@ int ocfs2_info_handle_freeinode(struct inode *inode,
>  
>  	o2info_set_request_filled(&oifi->ifi_req);
>  
> -	if (o2info_to_user(*oifi, req))
> -		goto bail;
> +	if (o2info_to_user(*oifi, req)) {
> +		status = -EFAULT;
> +		goto out_free;
> +	}
>  
>  	status = 0;
>  bail:
>  	if (status)
>  		o2info_set_request_error(&oifi->ifi_req, req);
> -
> +out_free:
>  	kfree(oifi);
>  out_err:
>  	return status;
> @@ -655,7 +616,7 @@ int ocfs2_info_handle_freefrag(struct inode *inode,
>  {
>  	u64 blkno = -1;
>  	char namebuf[40];
> -	int status = -EFAULT, type = GLOBAL_BITMAP_SYSTEM_INODE;
> +	int status, type = GLOBAL_BITMAP_SYSTEM_INODE;
>  
>  	struct ocfs2_info_freefrag *oiff;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> @@ -668,8 +629,10 @@ int ocfs2_info_handle_freefrag(struct inode *inode,
>  		goto out_err;
>  	}
>  
> -	if (o2info_from_user(*oiff, req))
> -		goto bail;
> +	if (o2info_from_user(*oiff, req)) {
> +		status = -EFAULT;
> +		goto out_free;
> +	}
>  	/*
>  	 * chunksize from userspace should be power of 2.
>  	 */
> @@ -708,14 +671,14 @@ int ocfs2_info_handle_freefrag(struct inode *inode,
>  
>  	if (o2info_to_user(*oiff, req)) {
>  		status = -EFAULT;
> -		goto bail;
> +		goto out_free;
>  	}
>  
>  	status = 0;
>  bail:
>  	if (status)
>  		o2info_set_request_error(&oiff->iff_req, req);
> -
> +out_free:
>  	kfree(oiff);
>  out_err:
>  	return status;
> @@ -724,23 +687,17 @@ out_err:
>  int ocfs2_info_handle_unknown(struct inode *inode,
>  			      struct ocfs2_info_request __user *req)
>  {
> -	int status = -EFAULT;
>  	struct ocfs2_info_request oir;
>  
>  	if (o2info_from_user(oir, req))
> -		goto bail;
> +		return -EFAULT;
>  
>  	o2info_clear_request_filled(&oir);
>  
>  	if (o2info_to_user(oir, req))
> -		goto bail;
> +		return -EFAULT;
>  
> -	status = 0;
> -bail:
> -	if (status)
> -		o2info_set_request_error(&oir, req);
> -
> -	return status;
> +	return 0;
>  }
>  
>  /*
> 
> -- 
> Ben Hutchings
> If at first you don't succeed, you're doing about average.



-- 

Life's Little Instruction Book #94

	"Make it a habit to do nice things for people who 
	 will never find out."

			http://www.jlbec.org/
			jlbec at evilplan.org

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [Ocfs2-devel] [PATCH] ocfs2: Do not write error flag to user structure we cannot copy from/to
  2013-11-07 11:22 ` Joel Becker
@ 2013-11-08  1:21   ` Ben Hutchings
  0 siblings, 0 replies; 3+ messages in thread
From: Ben Hutchings @ 2013-11-08  1:21 UTC (permalink / raw)
  To: ocfs2-devel

On Thu, 2013-11-07 at 03:22 -0800, Joel Becker wrote:
> On Sun, Oct 27, 2013 at 08:18:02PM +0000, Ben Hutchings wrote:
> > If we failed to copy from the structure, writing back the flags leaks
> > 31 bits of kernel memory (the rest of the ir_flags field).
> > 
> > In any case, if we cannot copy from/to the structure, why should we
> > expect putting just the flags to work?
> 
> The first issue could be fixed; we could clear the flags.  The second
> issue is what matters.  If we're getting EFAULT, we're not going to be
> moving any flags around.  We should just return the EFAULT and be done
> with it.
>  
> > Also make sure ocfs2_info_handle_freeinode() returns the right error
> > code if the copy_to_user() fails.
> > 
> > Compile-tested only.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Cc: stable at vger.kernel.org
> > Fixes: ddee5cdb70e6 ('Ocfs2: Add new OCFS2_IOC_INFO ioctl for ocfs2 v8.')
> 
> I can't recommend this for stable if it hasn't actually been tested.

I wouldn't expect you to apply it without testing!  But if it is
correct, then it should go to stable.

> There is no exposure; as you point out, the put_user() will fail if the
> get_user() is failing.

This is true for a program that accidentally used an invalid pointer.
But a malicious program can have one thread manipulating memory mappings
while the other thread runs the ioctl(), so that only the put_user()
succeeds.

> Send me a version without stable and I'll Ack it.
[...]

I believe this meets the criteria for stable.  It is your perogative as
maintainer to remove the cc if you disagree.

Ben.

-- 
Ben Hutchings
Horngren's Observation:
                   Among economists, the real world is often a special case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 828 bytes
Desc: This is a digitally signed message part
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20131108/ef5097f7/attachment.bin 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2013-11-08  1:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-27 20:18 [Ocfs2-devel] [PATCH] ocfs2: Do not write error flag to user structure we cannot copy from/to Ben Hutchings
2013-11-07 11:22 ` Joel Becker
2013-11-08  1:21   ` Ben Hutchings

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.