Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] dm snap: TOCTOU race in snapshot_ctr() allows upgrade_mode() UAF on s->origin --
commit b2c589f4852e6503996057072cf65216f108da3f Author: Igor Putko <[email protected]> dm snap: fix use-after-free when origin and COW devices match This commit resolves cow_path via dm_devt_from_path() in snapshot_ctr() and compares the resulting dev_t against s->origin->bdev->bd_dev before calling dm_get_device(), avoiding a use-after-free when upgrade_mode() replaces the origin table_device. > diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c > index 797c432594d86..ecda207e800d7 100644 > --- a/drivers/md/dm-snap.c > +++ b/drivers/md/dm-snap.c > @@ -1234,6 +1234,7 @@ static int snapshot_ctr(struct dm_target *ti, unsigned > int argc, char **argv) > int i; > int r = -EINVAL; > char *origin_path, *cow_path; > + dev_t cow_dev; > unsigned int args_used, num_flush_bios = 1; > blk_mode_t origin_mode = BLK_OPEN_READ; > > @@ -1276,15 +1277,21 @@ static int snapshot_ctr(struct dm_target *ti, > unsigned int argc, char **argv) > argv++; > argc--; > > - r = dm_get_device(ti, cow_path, dm_table_get_mode(ti->table), &s->cow); > + r = dm_devt_from_path(cow_path, &cow_dev); > if (r) { > ti->error = "Cannot get COW device"; > goto bad_cow; > } > - if (s->cow->bdev && s->cow->bdev == s->origin->bdev) { > + if (cow_dev == s->origin->bdev->bd_dev) { > ti->error = "COW device cannot be the same as origin device"; > r = -EINVAL; > - goto bad_store; > + goto bad_cow; > + } > + > + r = dm_get_device(ti, cow_path, dm_table_get_mode(ti->table), &s->cow); > + if (r) { > + ti->error = "Cannot get COW device"; > + goto bad_cow; > } [Severity: Critical] Could a race between the two path lookups still trigger the use-after-free on s->origin and leak the device reference? When cow_path is a filesystem path rather than a major:minor string, snapshot_ctr() first resolves it via dm_devt_from_path() -> lookup_bdev() -> kern_path(), reads inode->i_rdev, and drops the path reference via path_put(). Immediately afterwards, dm_get_device() resolves cow_path a second time: drivers/md/dm-table.c:dm_get_device() { r = dm_devt_from_path(path, &dev); if (r) return r; ... dd = find_device(&t->devices, dev); if (!dd) { ... } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { r = upgrade_mode(dd, mode, t->md); if (r) return r; } } If cow_path (such as a symlink or block device node) is modified between the first lookup in snapshot_ctr() and the second lookup in dm_get_device() to point to the same block device as s->origin, find_device() matches the existing dm_dev_internal entry for s->origin and calls upgrade_mode(): drivers/md/dm-table.c:upgrade_mode() { old_dev = dd->dm_dev; r = dm_get_table_device(md, dd->dm_dev->bdev->bd_dev, dd->dm_dev->mode | new_mode, &new_dev); if (r) return r; dd->dm_dev = new_dev; dm_put_table_device(md, old_dev); } Because the post-open check comparing s->cow->bdev and s->origin->bdev was removed, wouldn't snapshot_ctr() continue execution with s->origin pointing to the freed old_dev, dereferencing freed slab memory in register_snapshot() (via snap->origin->bdev) or dm_put_device(), while also leaking the reference on dd in ti->table->devices when dm_put_device(ti, s->origin) fails to find the replaced dm_dev? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
