On 4/20/23 21:17, Sergei Shtepa wrote: > Subject: > Re: [PATCH v3 03/11] documentation: Block Devices Snapshots Module > From: > Sergei Shtepa <sergei.shtepa@xxxxxxxxx> > Date: > 4/20/23, 21:17 > > To: > Donald Buczek <buczek@xxxxxxxxxxxxx>, axboe@xxxxxxxxx, hch@xxxxxxxxxxxxx, corbet@xxxxxxx, snitzer@xxxxxxxxxx > CC: > viro@xxxxxxxxxxxxxxxxxx, brauner@xxxxxxxxxx, willy@xxxxxxxxxxxxx, kch@xxxxxxxxxx, martin.petersen@xxxxxxxxxx, vkoul@xxxxxxxxxx, ming.lei@xxxxxxxxxx, gregkh@xxxxxxxxxxxxxxxxxxx, linux-block@xxxxxxxxxxxxxxx, linux-doc@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx > > > > On 4/20/23 16:44, Donald Buczek wrote: >> Subject: >> Re: [PATCH v3 03/11] documentation: Block Devices Snapshots Module >> From: >> Donald Buczek <buczek@xxxxxxxxxxxxx> >> Date: >> 4/20/23, 16:44 >> >> To: >> Sergei Shtepa <sergei.shtepa@xxxxxxxxx>, axboe@xxxxxxxxx, hch@xxxxxxxxxxxxx, corbet@xxxxxxx, snitzer@xxxxxxxxxx >> CC: >> viro@xxxxxxxxxxxxxxxxxx, brauner@xxxxxxxxxx, willy@xxxxxxxxxxxxx, kch@xxxxxxxxxx, martin.petersen@xxxxxxxxxx, vkoul@xxxxxxxxxx, ming.lei@xxxxxxxxxx, gregkh@xxxxxxxxxxxxxxxxxxx, linux-block@xxxxxxxxxxxxxxx, linux-doc@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx >> >> >> On 4/19/23 21:42, Donald Buczek wrote: >>> Dear Sergei, >>> >>> On 4/19/23 15:05, Sergei Shtepa wrote: >>>> [...] >>>> >>>> Patches in attach and https://github.com/SergeiShtepa/linux/tree/blksnap-master >>> Thanks. I can confirm that this fixes the reported problem and I no longer can trigger the UAF. 😄 >>> >>> Tested-Bny: Donald Buczek <buczek@xxxxxxxxxxxxx> >>> >>> Maybe you can add me to the cc list for v4 as I'm not subscribed to the lists. >> >> Sorry, found another one. Reproducer: >> >> ===== >> #! /bin/bash >> set -xe >> modprobe blksnap >> test -e /scratch/local/test.dat || fallocate -l 1G /scratch/local/test.dat >> s=$(blksnap snapshot_create -d /dev/vdb) >> blksnap snapshot_appendstorage -i $s -f /scratch/local/test.dat >> blksnap snapshot_take -i $s >> s2=$(blksnap snapshot_create -d /dev/vdb) >> blksnap snapshot_destroy -i $s2 >> blksnap snapshot_destroy -i $s >> ===== >> >> >> [20382.402921] blksnap-snapshot: Snapshot ff1c54f1-3e8c-4c99-bb26-35e82dc1c9fa was created >> [20382.535933] blksnap-image: Create snapshot image device for original device [253:16] >> [20382.542405] blksnap-snapshot: Snapshot ff1c54f1-3e8c-4c99-bb26-35e82dc1c9fa was taken successfully >> [20382.572564] blksnap-snapshot: Snapshot 4b2d571d-9a24-419d-96c2-8d64a07c4966 was created >> [20382.600521] blksnap-snapshot: Destroy snapshot 4b2d571d-9a24-419d-96c2-8d64a07c4966 >> [20382.602373] blksnap-snapshot: Release snapshot 4b2d571d-9a24-419d-96c2-8d64a07c4966 >> [20382.722137] blksnap-snapshot: Destroy snapshot ff1c54f1-3e8c-4c99-bb26-35e82dc1c9fa >> [20382.724033] blksnap-snapshot: Release snapshot ff1c54f1-3e8c-4c99-bb26-35e82dc1c9fa >> [20382.725850] ================================================================== >> [20382.727641] BUG: KASAN: wild-memory-access in snapshot_free+0x73/0x170 [blksnap] >> [20382.729326] Write of size 8 at addr dead000000000108 by task blksnap/8297 >> ... > Great! Thanks. > > There is no protection against re-adding a block device to the snapshot. > I'll take care of it. > Hi! I think the fix turned out to be quite beautiful. Now you will get an error "Device or resource busy". Fix in attach and on github. Link: https://github.com/SergeiShtepa/linux/commit/43a5d3dd9858f092b734187b6a62ce75acaa47c7
diff --git a/drivers/block/blksnap/snapshot.c b/drivers/block/blksnap/snapshot.c index 1f28ff59d762..8063cc4b929e 100644 --- a/drivers/block/blksnap/snapshot.c +++ b/drivers/block/blksnap/snapshot.c @@ -28,7 +28,7 @@ static void snapshot_free(struct kref *kref) tracker = list_first_entry(&snapshot->trackers, struct tracker, link); - list_del(&tracker->link); + list_del_init(&tracker->link); tracker_release_snapshot(tracker); tracker_put(tracker); } @@ -160,14 +160,17 @@ int snapshot_add_device(const uuid_t *id, struct tracker *tracker) } } if (!ret) { - tracker_get(tracker); - list_add_tail(&tracker->link, &snapshot->trackers); + if (list_empty(&tracker->link)) { + tracker_get(tracker); + list_add_tail(&tracker->link, &snapshot->trackers); + } else + ret = -EBUSY; } up_write(&snapshot->rw_lock); snapshot_put(snapshot); - return 0; + return ret; } int snapshot_destroy(const uuid_t *id) diff --git a/drivers/block/blksnap/tracker.c b/drivers/block/blksnap/tracker.c index d98048dc5bed..008cc7f0f81e 100644 --- a/drivers/block/blksnap/tracker.c +++ b/drivers/block/blksnap/tracker.c @@ -79,6 +79,7 @@ static struct blkfilter *tracker_attach(struct block_device *bdev) return ERR_PTR(-ENOMEM); } + mutex_init(&tracker->ctl_lock); INIT_LIST_HEAD(&tracker->link); kref_init(&tracker->kref); tracker->dev_id = bdev->bd_dev; @@ -234,12 +235,17 @@ static int (*const ctl_table[])(struct tracker *tracker, static int tracker_ctl(struct blkfilter *flt, const unsigned int cmd, __u8 __user *buf, __u32 *plen) { + int ret = 0; struct tracker *tracker = container_of(flt, struct tracker, filter); if (cmd > ARRAY_SIZE(ctl_table)) return -ENOTTY; - return ctl_table[cmd](tracker, buf, plen); + mutex_lock(&tracker->ctl_lock); + ret = ctl_table[cmd](tracker, buf, plen); + mutex_unlock(&tracker->ctl_lock); + + return ret; } static struct blkfilter_operations tracker_ops = { diff --git a/drivers/block/blksnap/tracker.h b/drivers/block/blksnap/tracker.h index d0972994d528..dbf8295f9518 100644 --- a/drivers/block/blksnap/tracker.h +++ b/drivers/block/blksnap/tracker.h @@ -19,6 +19,9 @@ struct diff_area; * * @filter: * The block device filter structure. + * @ctl_lock: + * The mutex blocks simultaneous management of the tracker from different + * treads. * @link: * List header. Allows to combine trackers into a list in a snapshot. * @kref: @@ -41,6 +44,7 @@ struct diff_area; */ struct tracker { struct blkfilter filter; + struct mutex ctl_lock; struct list_head link; struct kref kref; dev_t dev_id;