[RFC,0/6,v2] block: Add config option to not allow writing to mounted devices

Message

Jan Kara July 4, 2023, 12:56 p.m. UTC

Hello!

This is second version of the patches to add config option to not allow writing
to mounted block devices. For motivation why this is interesting see patch 1/6.
I've been testing the patches more extensively this time and I've found couple
of things that get broken by disallowing writes to mounted block devices:
1) Bind mounts get broken because get_tree_bdev() / mount_bdev() first try to
   claim the bdev before searching whether it is already mounted. Patch 6
   reworks the mount code to avoid this problem.
2) btrfs mounting is likely having the same problem as 1). It should be fixable
   AFAICS but for now I've left it alone until we settle on the rest of the
   series.
3) "mount -o loop" gets broken because util-linux keeps the loop device open
   read-write when attempting to mount it. Hopefully fixable within util-linux.
4) resize2fs online resizing gets broken because it tries to open the block
   device read-write only to call resizing ioctl. Trivial to fix within
   e2fsprogs.

Likely there will be other breakage I didn't find yet but overall the breakage
looks minor enough that the option might be useful. Definitely good enough
for syzbot fuzzing and likely good enough for hardening of systems with
more tightened security.

This patch set is based on the patches making blkdev_get_by_*() return
bdev_handle [1].

Changes since v1:
* Added kernel cmdline argument to toggle whether writing to mounted block
  devices is allowed or not
* Fixed handling of partitions
* Limit write blocking only to devices open with explicit BLK_OPEN_BLOCK_WRITES
  flag

								Honza

[1] https://lore.kernel.org/all/[e-mail]

Previous versions:
v1: https://lore.kernel.org/all/[e-mail]

Comments

Christian Brauner July 4, 2023, 1:40 p.m. UTC | #1

On Tue, Jul 04, 2023 at 02:56:48PM +0200, Jan Kara wrote:
> Hello!
> 
> This is second version of the patches to add config option to not allow writing
> to mounted block devices. For motivation why this is interesting see patch 1/6.
> I've been testing the patches more extensively this time and I've found couple
> of things that get broken by disallowing writes to mounted block devices:
> 1) Bind mounts get broken because get_tree_bdev() / mount_bdev() first try to
>    claim the bdev before searching whether it is already mounted. Patch 6
>    reworks the mount code to avoid this problem.
> 2) btrfs mounting is likely having the same problem as 1). It should be fixable

It likely would. Note that I've got a series to port btrfs to the new
mount api that I sent out which changes btrfs mounting quite
significantly.

Mike Fleetwood July 5, 2023, 12:27 p.m. UTC | #2

On Tue, 4 Jul 2023 at 13:57, Jan Kara <[e-mail]> wrote:
>
> Hello!
>
> This is second version of the patches to add config option to not allow writing
> to mounted block devices. For motivation why this is interesting see patch 1/6.
> I've been testing the patches more extensively this time and I've found couple
> of things that get broken by disallowing writes to mounted block devices:
> 1) Bind mounts get broken because get_tree_bdev() / mount_bdev() first try to
>    claim the bdev before searching whether it is already mounted. Patch 6
>    reworks the mount code to avoid this problem.
> 2) btrfs mounting is likely having the same problem as 1). It should be fixable
>    AFAICS but for now I've left it alone until we settle on the rest of the
>    series.
> 3) "mount -o loop" gets broken because util-linux keeps the loop device open
>    read-write when attempting to mount it. Hopefully fixable within util-linux.
> 4) resize2fs online resizing gets broken because it tries to open the block
>    device read-write only to call resizing ioctl. Trivial to fix within
>    e2fsprogs.
>
> Likely there will be other breakage I didn't find yet but overall the breakage
> looks minor enough that the option might be useful. Definitely good enough
> for syzbot fuzzing and likely good enough for hardening of systems with
> more tightened security.

5) Online e2label will break because it directly writes to the ext2/3/4
   superblock while the FS is mounted to set the new label.  Ext4 driver
   will have to implement the SETFSLABEL ioctl() and e2label will have
   to use it, matching what happens for online labelling of btrfs and
   xfs.

Thanks,
Mike

Jan Kara Aug. 14, 2023, 4:39 p.m. UTC | #3

On Wed 05-07-23 13:27:03, Mike Fleetwood wrote:
> On Tue, 4 Jul 2023 at 13:57, Jan Kara <[e-mail]> wrote:
> >
> > Hello!
> >
> > This is second version of the patches to add config option to not allow writing
> > to mounted block devices. For motivation why this is interesting see patch 1/6.
> > I've been testing the patches more extensively this time and I've found couple
> > of things that get broken by disallowing writes to mounted block devices:
> > 1) Bind mounts get broken because get_tree_bdev() / mount_bdev() first try to
> >    claim the bdev before searching whether it is already mounted. Patch 6
> >    reworks the mount code to avoid this problem.
> > 2) btrfs mounting is likely having the same problem as 1). It should be fixable
> >    AFAICS but for now I've left it alone until we settle on the rest of the
> >    series.
> > 3) "mount -o loop" gets broken because util-linux keeps the loop device open
> >    read-write when attempting to mount it. Hopefully fixable within util-linux.
> > 4) resize2fs online resizing gets broken because it tries to open the block
> >    device read-write only to call resizing ioctl. Trivial to fix within
> >    e2fsprogs.
> >
> > Likely there will be other breakage I didn't find yet but overall the breakage
> > looks minor enough that the option might be useful. Definitely good enough
> > for syzbot fuzzing and likely good enough for hardening of systems with
> > more tightened security.
> 
> 5) Online e2label will break because it directly writes to the ext2/3/4
>    superblock while the FS is mounted to set the new label.  Ext4 driver
>    will have to implement the SETFSLABEL ioctl() and e2label will have
>    to use it, matching what happens for online labelling of btrfs and
>    xfs.

Thanks, added to the description.

								Honza