Re: [PATCH 1/2] Fail I/O to thin pool devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Nack.

I don't see the security issue; how is this any different from running the thin tools on any incorrect device?  Or even the data device that the pool is mirroring.  In general the thin tools don't modify the metadata they're running on.  If you know of a security issue with the thin tools please let me know.


On Tue, Feb 7, 2023 at 7:56 AM Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> wrote:
A thin pool device currently just passes all I/O to its origin device,
but this is a footgun: the user might not realize that tools that
operate on thin pool metadata must operate on the metadata volume.  This
could have security implications.

Fix this by failing all I/O to thin pool devices.

Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
---
 drivers/md/dm-thin.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 64cfcf46881dc5d87d5dfdb5650ba9babd32cd31..d85fdbd782ae5426003c99a4b4bf53818cc85efa 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -3405,19 +3405,14 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)

 static int pool_map(struct dm_target *ti, struct bio *bio)
 {
-       int r;
-       struct pool_c *pt = ti->private;
-       struct pool *pool = pt->pool;
-
        /*
-        * As this is a singleton target, ti->begin is always zero.
+        * Previously, access to the pool was passed down to the origin device.
+        * However, this turns out to be error-prone: if the user runs any of
+        * the thin tools on the pool device, the tools could wind up parsing
+        * potentially attacker-controlled data.  This mistake has actually
+        * happened in practice.  Therefore, fail all I/O on the pool device.
         */
-       spin_lock_irq(&pool->lock);
-       bio_set_dev(bio, pt->data_dev->bdev);
-       r = DM_MAPIO_REMAPPED;
-       spin_unlock_irq(&pool->lock);
-
-       return r;
+       return -EIO;
 }

 static int maybe_resize_data_dev(struct dm_target *ti, bool *need_commit)
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel

[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux