Re: [PATCH] Btrfs: fix use-after-free when cloning a trailing file hole

Re: [PATCH] Btrfs: fix use-after-free when cloning a trailing file hole

[Chris Mason]

On 06/29/2014 03:43 PM, Filipe David Borba Manana wrote:
> The transaction handle was being used after being freed.
> 
> Cc: Chris Mason <clm@fb.com>
> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
> ---
>  fs/btrfs/ioctl.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 02dc64b..2562dc7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3565,12 +3565,10 @@ process_slot:
>  			btrfs_end_transaction(trans, root);
>  			goto out;
>  		}
> -		ret = clone_finish_inode_update(trans, inode, destoff + len,
> -						destoff, olen);
> -		if (ret)
> -			goto out;
>  		clone_update_extent_map(inode, trans, path, NULL, last_dest_end,
>  					destoff + len - last_dest_end);
> +		ret = clone_finish_inode_update(trans, inode, destoff + len,
> +						destoff, olen);
>  	}
>  
>  out:
> 

What about the path?  It has been released, so it should either be NULL
or we have other problems ;)

-chris

Re: [PATCH] Btrfs: fix use-after-free when cloning a trailing file hole

[Filipe David Manana]

On Sun, Jun 29, 2014 at 8:23 PM, Chris Mason <clm@fb.com> wrote:
> On 06/29/2014 03:43 PM, Filipe David Borba Manana wrote:
>> The transaction handle was being used after being freed.
>>
>> Cc: Chris Mason <clm@fb.com>
>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>> ---
>>  fs/btrfs/ioctl.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 02dc64b..2562dc7 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3565,12 +3565,10 @@ process_slot:
>>                       btrfs_end_transaction(trans, root);
>>                       goto out;
>>               }
>> -             ret = clone_finish_inode_update(trans, inode, destoff + len,
>> -                                             destoff, olen);
>> -             if (ret)
>> -                     goto out;
>>               clone_update_extent_map(inode, trans, path, NULL, last_dest_end,
>>                                       destoff + len - last_dest_end);
>> +             ret = clone_finish_inode_update(trans, inode, destoff + len,
>> +                                             destoff, olen);
>>       }
>>
>>  out:
>>
>
> What about the path?  It has been released, so it should either be NULL
> or we have other problems ;)

Hi Chris,

Not needed. clone_update_extent_map ignores path if its 4th argument
is NULL (which is the case here).
Either way it's more clear if path is passed to that function as NULL
too. I'll update it for clarity.

>
> -chris



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

[PATCH] Btrfs: fix use-after-free when cloning a trailing file hole

[Filipe David Borba Manana]

The transaction handle was being used after being freed.

Cc: Chris Mason <clm@fb.com>
Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---
 fs/btrfs/ioctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 02dc64b..2562dc7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3565,12 +3565,10 @@ process_slot:
 			btrfs_end_transaction(trans, root);
 			goto out;
 		}
-		ret = clone_finish_inode_update(trans, inode, destoff + len,
-						destoff, olen);
-		if (ret)
-			goto out;
 		clone_update_extent_map(inode, trans, path, NULL, last_dest_end,
 					destoff + len - last_dest_end);
+		ret = clone_finish_inode_update(trans, inode, destoff + len,
+						destoff, olen);
 	}
 
 out:
-- 
1.9.1

[PATCH v2] Btrfs: fix use-after-free when cloning a trailing file hole

[Filipe David Borba Manana]

The transaction handle was being used after being freed.

Cc: Chris Mason <clm@fb.com>
Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---

V2: Removed file extent item argument to clone_update_extent_map() for
    more clarity.

 fs/btrfs/ioctl.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 02dc64b..2a99f49 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3142,7 +3142,6 @@ out:
 static void clone_update_extent_map(struct inode *inode,
 				    const struct btrfs_trans_handle *trans,
 				    const struct btrfs_path *path,
-				    struct btrfs_file_extent_item *fi,
 				    const u64 hole_offset,
 				    const u64 hole_len)
 {
@@ -3157,7 +3156,11 @@ static void clone_update_extent_map(struct inode *inode,
 		return;
 	}
 
-	if (fi) {
+	if (path) {
+		struct btrfs_file_extent_item *fi;
+
+		fi = btrfs_item_ptr(path->nodes[0], path->slots[0],
+				    struct btrfs_file_extent_item);
 		btrfs_extent_item_to_extent_map(inode, path, fi, false, em);
 		em->generation = -1;
 		if (btrfs_file_extent_type(path->nodes[0], fi) ==
@@ -3511,18 +3514,15 @@ process_slot:
 					    btrfs_item_ptr_offset(leaf, slot),
 					    size);
 				inode_add_bytes(inode, datal);
-				extent = btrfs_item_ptr(leaf, slot,
-						struct btrfs_file_extent_item);
 			}
 
 			/* If we have an implicit hole (NO_HOLES feature). */
 			if (drop_start < new_key.offset)
 				clone_update_extent_map(inode, trans,
-						path, NULL, drop_start,
+						NULL, drop_start,
 						new_key.offset - drop_start);
 
-			clone_update_extent_map(inode, trans, path,
-						extent, 0, 0);
+			clone_update_extent_map(inode, trans, path, 0, 0);
 
 			btrfs_mark_buffer_dirty(leaf);
 			btrfs_release_path(path);
@@ -3565,12 +3565,10 @@ process_slot:
 			btrfs_end_transaction(trans, root);
 			goto out;
 		}
+		clone_update_extent_map(inode, trans, NULL, last_dest_end,
+					destoff + len - last_dest_end);
 		ret = clone_finish_inode_update(trans, inode, destoff + len,
 						destoff, olen);
-		if (ret)
-			goto out;
-		clone_update_extent_map(inode, trans, path, NULL, last_dest_end,
-					destoff + len - last_dest_end);
 	}
 
 out:
-- 
1.9.1