[PATCH] Fix a use-after-free in the volumes code.

[PATCH] Fix a use-after-free in the volumes code.

[Adam Buchbinder]

When a struct btrfs_fs_devices was being torn down by
btrfs_close_devices(), there was an invalidated pointer in the global
list fs_uuids which still pointed to it; if a device was closed and
then reopened (which btrfs-convert does), freed memory would be
accessed.

This was found using ThreadSanitizer (pretty much doing what
AddressSanitizer would, but not exiting after the first failure).
To reproduce, build with -fsanitize=thread and run 'make test'.
Representative output is below.

This change makes the current tests TSan-clean.

WARNING: ThreadSanitizer: heap-use-after-free (pid=29161)
  Read of size 8 at 0x7d180000eee0 by main thread:
    #0 memcmp ??:0
    #1 find_fsid .../volumes.c:81
    #2 device_list_add .../volumes.c:95
    #3 btrfs_scan_one_device .../volumes.c:259
    #4 btrfs_scan_fs_devices .../disk-io.c:1002
    #5 __open_ctree_fd .../disk-io.c:1090
    #6 open_ctree_fd .../disk-io.c:1191
    #7 do_convert .../btrfs-convert.c:2317
    #8 main .../btrfs-convert.c:2745

  Previous write of size 8 at 0x7d180000eee0 by main thread:
    #0 free ??:0
    #1 btrfs_close_devices .../volumes.c:191
    #2 close_ctree .../disk-io.c:1401
    #3 do_convert .../btrfs-convert.c:2300
    #4 main .../btrfs-convert.c:2745

  Location is heap block of size 96 at 0x7d180000eee0 allocated by main thread:
    #0 calloc ??:0 (exe+0x00000002acc6)
    #1 device_list_add .../volumes.c:97
    #2 btrfs_scan_one_device .../volumes.c:259
    #3 btrfs_scan_fs_devices .../disk-io.c:1002
    #4 __open_ctree_fd .../disk-io.c:1090
    #5 open_ctree_fd .../disk-io.c:1191
    #6 do_convert .../btrfs-convert.c:2256
    #7 main .../btrfs-convert.c:2745

Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
---
 volumes.c | 1 +
 volumes.h | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/volumes.c b/volumes.c
index a61928c..36f6050 100644
--- a/volumes.c
+++ b/volumes.c
@@ -188,6 +188,7 @@ again:
 		goto again;
 	}
 
+	list_del(&fs_devices->list);
 	free(fs_devices);
 	return 0;
 }
diff --git a/volumes.h b/volumes.h
index b1ff3d0..2e960b5 100644
--- a/volumes.h
+++ b/volumes.h
@@ -67,7 +67,7 @@ struct btrfs_device {
 struct btrfs_fs_devices {
 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
 
-	/* the device with this id has the most recent coyp of the super */
+	/* the device with this id has the most recent copy of the super */
 	u64 latest_devid;
 	u64 latest_trans;
 	u64 lowest_devid;
-- 
2.0.0.526.g5318336

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-

Re: [PATCH] Fix a use-after-free in the volumes code.

[Satoru Takeuchi]

Hi Adam,

(2014/06/14 8:43), Adam Buchbinder wrote:
> When a struct btrfs_fs_devices was being torn down by
> btrfs_close_devices(), there was an invalidated pointer in the global
> list fs_uuids which still pointed to it; if a device was closed and
> then reopened (which btrfs-convert does), freed memory would be
> accessed.

It looks good to me.

Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

I confirmed ...

 - btrfs_fs_devices is linked to fs_uuids in device_list_add().
 - However, there is no list_del(btrfs_fs_devices) from
   fs_uuids at all.
 - There is at least one use-after-free of btrfs_fs_devices
   as you said (btrfs-convert).

P.S.
I suggest you to add "btrfs-progs: " or "btrfs:" tag to mail subject
from the next time.

Thanks,
Satoru

> 
> This was found using ThreadSanitizer (pretty much doing what
> AddressSanitizer would, but not exiting after the first failure).
> To reproduce, build with -fsanitize=thread and run 'make test'.
> Representative output is below.
> 
> This change makes the current tests TSan-clean.
> 
> WARNING: ThreadSanitizer: heap-use-after-free (pid=29161)
>    Read of size 8 at 0x7d180000eee0 by main thread:
>      #0 memcmp ??:0
>      #1 find_fsid .../volumes.c:81
>      #2 device_list_add .../volumes.c:95
>      #3 btrfs_scan_one_device .../volumes.c:259
>      #4 btrfs_scan_fs_devices .../disk-io.c:1002
>      #5 __open_ctree_fd .../disk-io.c:1090
>      #6 open_ctree_fd .../disk-io.c:1191
>      #7 do_convert .../btrfs-convert.c:2317
>      #8 main .../btrfs-convert.c:2745
> 
>    Previous write of size 8 at 0x7d180000eee0 by main thread:
>      #0 free ??:0
>      #1 btrfs_close_devices .../volumes.c:191
>      #2 close_ctree .../disk-io.c:1401
>      #3 do_convert .../btrfs-convert.c:2300
>      #4 main .../btrfs-convert.c:2745
> 
>    Location is heap block of size 96 at 0x7d180000eee0 allocated by main thread:
>      #0 calloc ??:0 (exe+0x00000002acc6)
>      #1 device_list_add .../volumes.c:97
>      #2 btrfs_scan_one_device .../volumes.c:259
>      #3 btrfs_scan_fs_devices .../disk-io.c:1002
>      #4 __open_ctree_fd .../disk-io.c:1090
>      #5 open_ctree_fd .../disk-io.c:1191
>      #6 do_convert .../btrfs-convert.c:2256
>      #7 main .../btrfs-convert.c:2745
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
> ---
>   volumes.c | 1 +
>   volumes.h | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/volumes.c b/volumes.c
> index a61928c..36f6050 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -188,6 +188,7 @@ again:
>   		goto again;
>   	}
>   
> +	list_del(&fs_devices->list);
>   	free(fs_devices);
>   	return 0;
>   }
> diff --git a/volumes.h b/volumes.h
> index b1ff3d0..2e960b5 100644
> --- a/volumes.h
> +++ b/volumes.h
> @@ -67,7 +67,7 @@ struct btrfs_device {
>   struct btrfs_fs_devices {
>   	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
>   
> -	/* the device with this id has the most recent coyp of the super */
> +	/* the device with this id has the most recent copy of the super */
>   	u64 latest_devid;
>   	u64 latest_trans;
>   	u64 lowest_devid;
>