When delete partition executes concurrently with IOs issue,
it may cause use-after-free on part in disk_map_sector_rcu()
as following:
blk_account_io_start(req1) delete_partition blk_account_io_start(req2)
rcu_read_lock()
disk_map_sector_rcu
part = rcu_dereference(ptbl->part[4])
rcu_assign_pointer(ptbl->part[4], NULL);
rcu_assign_pointer(ptbl->last_lookup, NULL);
rcu_assign_pointer(ptbl->last_lookup, part);
hd_struct_kill(part)
!hd_struct_try_get
part = &rq->rq_disk->part0;
rcu_read_unlock()
__delete_partition
call_rcu
rcu_read_lock
disk_map_sector_rcu
part = rcu_dereference(ptbl->last_lookup);
delete_partition_work_fn
free(part)
hd_struct_try_get(part)
BUG_ON use-after-free
req1 try to get 'ptbl->part[4]', while the part is beening
deleted. Although the delete_partition() try to set last_lookup
as NULL, req1 can overwrite it as 'part[4]' again.
After calling call_rcu() and free() for the part, req2 can
get the part by last_lookup, resulting in use after free.
In fact, this bug has been reported by syzbot:
https://lkml.org/lkml/2019/1/4/357
We fix the bug by rereading ptbl->part[i] after setting
last_lookup. If part[i] is NULL, that means the part will
be freed, we need to clear last_lookup.
Then we can make sure last_lookup have been set as NULL
before call_rcu(). Thus, others can not get the freed part.
Fixes: a6f23657d307 ("block: add one-hit cache for disk partition lookup")
Reported-by: Zheng Chuan <zhengchuan@xxxxxxxxxx>
Signed-off-by: Yufen Yu <yuyufen@xxxxxxxxxx>
---
block/genhd.c | 16 ++++++++++++++++
block/partition-generic.c | 7 +++++++
2 files changed, 23 insertions(+)
diff --git a/block/genhd.c b/block/genhd.c
index ff6268970ddc..39fa8999905f 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
part = rcu_dereference(ptbl->part[i]);
if (part && sector_in_part(part, sector)) {
+ /*
+ * After delete_partition() trying to delete this
+ * part and setting last_lookup as 'NULL', we can
+ * overwrite it here. Then, others may use the freed
+ * part by reading last_lookup.
+ *
+ * To avoiding the use-after-free, we reread the
+ * value of ptbl->part[i] here. If it has been set
+ * as 'NULL', that means the part will be freed.
+ * And we need to clear last_lookup also.
+ */
rcu_assign_pointer(ptbl->last_lookup, part);
+ part = rcu_dereference(ptbl->part[i]);
+ if (part == NULL) {
+ rcu_assign_pointer(ptbl->last_lookup, NULL);
+ break;
+ }
return part;
}
}
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1d20c9cf213f..1e0065ed6f02 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
return;
rcu_assign_pointer(ptbl->part[partno], NULL);
+ /*
+ * Without the memory barrier, disk_map_sector_rcu()
+ * may read the old value after overwriting the
+ * last_lookup. Then it can not clear last_lookup,
+ * which may cause use-after-free.
+ */
+ smp_mb();
rcu_assign_pointer(ptbl->last_lookup, NULL);
kobject_put(part->holder_dir);
device_del(part_to_dev(part));
--
2.17.2
