Skip to contentSkip to conversation

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

PATCH Superseded Filipe David Borba Manana 2 participants 4 messages Jun 29, 2014 Btrfs file system

lore
Share & tools
lore

Filipe David Borba Manana

author
#0 lore raw

PATCH 1/1 Btrfs: fix use-after-free when cloning a trailing file hole

The transaction handle was being used after being freed.

Cc Chris Mason <[email protected]>
Signed-off-by Filipe David Borba Manana <[email protected]>
---
 fs/btrfs/ioctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
fs/btrfs/ioctl.c +2 -4
index 02dc64b..2562dc7 100644
@@ -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:
Signature · 1 line
1.9.1
#1 lore raw
Quoted 18 lines · Filipe David Borba Manana

The transaction handle was being used after being freed.

Cc Chris Mason <[email protected]>
Signed-off-by Filipe David Borba Manana <[email protected]>
---
 fs/btrfs/ioctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
fs/btrfs/ioctl.c +0 -1
index 02dc64b..2562dc7 100644
@@ -3565,12 +3565,10 @@ process_slot:
 			btrfs_end_transaction(trans, root);
 			goto out;
 		}
-		ret = clone_finish_inode_update(trans, inode, destoff + len,

fs/btrfs/ioctl.c process_slot: · line 3574

Quoted 10 lines
fs/btrfs/ioctl.c +2 -3
-						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

author
#2 lore raw
Quoted 27 lines · Chris Mason
Quoted 26 lines · Filipe David Borba Manana

The transaction handle was being used after being freed.

Cc Chris Mason <[email protected]>
Signed-off-by Filipe David Borba Manana <[email protected]>
---
 fs/btrfs/ioctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
fs/btrfs/ioctl.c +2 -4
index 02dc64b..2562dc7 100644
@@ -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);
      }
Quoted 5 lines

fs/btrfs/ioctl.c process_slot: · line 3574

Quoted 2 lines
fs/btrfs/ioctl.c +0 -0
 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.

Quoted 2 lines

-chris

Signature · 5 lines
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."
author
#3 lore raw

PATCH v2 1/1 Btrfs: fix use-after-free when cloning a trailing file hole

In reply to Filipe David Borba Manana

The transaction handle was being used after being freed.

Cc Chris Mason <[email protected]>
Signed-off-by Filipe David Borba Manana <[email protected]>

---

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(-)
fs/btrfs/ioctl.c +9 -11
index 02dc64b..2a99f49 100644
@@ -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:
Signature · 1 line
1.9.1