[PATCH] btrfs: fix a potential double put bug and some related use-after-free bugs

* [PATCH] btrfs: fix a potential double put bug and some related use-after-free bugs
@ 2021-08-18  9:15 Wentao_Liang
  2021-08-18 10:46 ` David Sterba
  2021-08-18 12:11 ` Nikolay Borisov
  0 siblings, 2 replies; 3+ messages in thread
From: Wentao_Liang @ 2021-08-18  9:15 UTC (permalink / raw)
  To: clm; +Cc: josef, dsterba, linux-btrfs, linux-kernel, Wentao_Liang

In line 2955 (#1), "btrfs_put_block_group(cache);" drops the reference to
cache and may cause the cache to be released. However, in line 3014, the
cache is dropped again by the same put function (#4). Double putting the
cache can lead to an incorrect reference count.

Furthermore, according to the definition of btrfs_put_block_group() in fs/
btrfs/block-group.c, if the reference count of the cache is one at the
first put, it will be freed by kfree(). Using it again may result in the
use-after-free flaw. In fact, after the first put (line 2955), the cache
is also accessed in a few places (#2, #3), e.g., lines 2967, 2973, 2974,
….

We believe that the first put of the cache is unnecessary (#1).
We can fix the above bugs by removing the redundant
"btrfs_put_block_group(cache);" in line 2955 (#1).

2951         if (!list_empty(&cache->io_list)) {
...
2955             btrfs_put_block_group(cache);
				 //#1 the first drop to cache (unnecessary)
...
2957         }
...
2967         cache_save_setup(cache, trans, path); //#2 use the cache
...
2972          //#3 use the cache several times

2973         if (!ret && cache->disk_cache_state == BTRFS_DC_SETUP) {
2974             cache->io_ctl.inode = NULL;
2975             ret = btrfs_write_out_cache(trans, cache, path);
2976             if (ret == 0 && cache->io_ctl.inode) {
2977                 num_started++;
2978                 should_put = 0;
2979                 list_add_tail(&cache->io_list, io);
2980             } else {
...
2985                 ret = 0;
2986             }
2987         }
2988         if (!ret) {
2989             ret = update_block_group_item(trans, path, cache);
...
3003             if (ret == -ENOENT) {
...
3006                 ret = update_block_group_item(trans, path, cache);
3007             }
...
3010         }
3011
...
3013         if (should_put)
3014             btrfs_put_block_group(cache);
				//#4 the second drop to cache

Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
---
 fs/btrfs/block-group.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 9e7d9d0c763d..c510c821b1be 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2953,7 +2953,6 @@ int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans)
 			spin_unlock(&cur_trans->dirty_bgs_lock);
 			list_del_init(&cache->io_list);
 			btrfs_wait_cache_io(trans, cache, path);
-			btrfs_put_block_group(cache);
 			spin_lock(&cur_trans->dirty_bgs_lock);
 		}
 
-- 
2.25.1


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