Re: [PATCH 3/7] block: Add config option to not allow writing to mounted devices

[Jan Kara <jack@suse.cz>]

On Mon 06-11-23 15:47:54, Christian Brauner wrote:
> On Wed, Nov 01, 2023 at 06:43:08PM +0100, Jan Kara wrote:
> > Writing to mounted devices is dangerous and can lead to filesystem
> > corruption as well as crashes. Furthermore syzbot comes with more and
> > more involved examples how to corrupt block device under a mounted
> > filesystem leading to kernel crashes and reports we can do nothing
> > about. Add tracking of writers to each block device and a kernel cmdline
> > argument which controls whether other writeable opens to block devices
> > open with BLK_OPEN_RESTRICT_WRITES flag are allowed. We will make
> > filesystems use this flag for used devices.
> > 
> > Note that this effectively only prevents modification of the particular
> > block device's page cache by other writers. The actual device content
> > can still be modified by other means - e.g. by issuing direct scsi
> > commands, by doing writes through devices lower in the storage stack
> > (e.g. in case loop devices, DM, or MD are involved) etc. But blocking
> > direct modifications of the block device page cache is enough to give
> > filesystems a chance to perform data validation when loading data from
> > the underlying storage and thus prevent kernel crashes.
> > 
> > Syzbot can use this cmdline argument option to avoid uninteresting
> > crashes. Also users whose userspace setup does not need writing to
> > mounted block devices can set this option for hardening.
> > 
> > Link: https://lore.kernel.org/all/60788e5d-5c7c-1142-e554-c21d709acfd9@linaro.org
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> 
> A few minor tweaks I would do in-tree. Please see below.
> I know it's mostly stylistic that's why I would do it so there's no
> resend dance for non-technical reasons.

Whatever works best for you. I agree with the changes but please see my
comments below for some fixes.

> >  block/Kconfig             | 20 +++++++++++++
> >  block/bdev.c              | 62 ++++++++++++++++++++++++++++++++++++++-
> >  include/linux/blk_types.h |  1 +
> >  include/linux/blkdev.h    |  2 ++
> >  4 files changed, 84 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/Kconfig b/block/Kconfig
> > index f1364d1c0d93..ca04b657e058 100644
> > --- a/block/Kconfig
> > +++ b/block/Kconfig
> > @@ -78,6 +78,26 @@ config BLK_DEV_INTEGRITY_T10
> >  	select CRC_T10DIF
> >  	select CRC64_ROCKSOFT
> >  
> > +config BLK_DEV_WRITE_MOUNTED
> > +	bool "Allow writing to mounted block devices"
> > +	default y
> 
> Let's hope that this can become the default one day.

Yes, I'd hope as well but we need some tooling work (util-linux, e2fsprogs)
before that can happen.

> > +static void bdev_unblock_writes(struct block_device *bdev)
> > +{
> > +	bdev->bd_writers = 0;
> > +}
> > +
> > +static bool blkdev_open_compatible(struct block_device *bdev, blk_mode_t mode)
> 
> I would like to mirror our may_{open,create}() routines here and call
> this:
> 
>     bdev_may_open()
> 
> This is a well-known vfs pattern and also easy to understand for block
> devs as well.

Sure.

> > @@ -800,12 +834,21 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
> >  		goto abort_claiming;
> >  	if (!try_module_get(disk->fops->owner))
> >  		goto abort_claiming;
> > +	ret = -EBUSY;
> > +	if (!blkdev_open_compatible(bdev, mode))
> > +		goto abort_claiming;
> >  	if (bdev_is_partition(bdev))
> >  		ret = blkdev_get_part(bdev, mode);
> >  	else
> >  		ret = blkdev_get_whole(bdev, mode);
> >  	if (ret)
> >  		goto put_module;
> > +	if (!bdev_allow_write_mounted) {
> > +		if (mode & BLK_OPEN_RESTRICT_WRITES)
> > +			bdev_block_writes(bdev);
> > +		else if (mode & BLK_OPEN_WRITE)
> > +			bdev->bd_writers++;
> > +	}
> 
> I would like to move this to a tiny helper for clarity:
> 
> static void bdev_claim_write_access(struct block_device *bdev)
> {
>         if (!bdev_allow_write_mounted)

This should be the other way around.

>                 return;
> 
>         /* Claim exclusive or shared write access to the block device. */
>         if (mode & BLK_OPEN_RESTRICT_WRITES)
>                 bdev_block_writes(bdev);
>         else if (mode & BLK_OPEN_WRITE)
>                 bdev->bd_writers++;
> }
> 
> >  	if (holder) {
> >  		bd_finish_claiming(bdev, holder, hops);
> >  
> > @@ -901,6 +944,14 @@ void bdev_release(struct bdev_handle *handle)
> >  		sync_blockdev(bdev);
> >  
> >  	mutex_lock(&disk->open_mutex);
> > +	if (!bdev_allow_write_mounted) {
> > +		/* The exclusive opener was blocking writes? Unblock them. */
> > +		if (handle->mode & BLK_OPEN_RESTRICT_WRITES)
> > +			bdev_unblock_writes(bdev);
> > +		else if (handle->mode & BLK_OPEN_WRITE)
> > +			bdev->bd_writers--;
> > +	}
> 
> static void bdev_yield_write_access(struct block_device *bdev)
> {
>         if (!bdev_allow_write_mounted)

And this as well.

>                 return;
> 
>         /* Yield exclusive or shared write access. */
>         if (handle->mode & BLK_OPEN_RESTRICT_WRITES)
>                 bdev_unblock_writes(bdev);
>         else if (handle->mode & BLK_OPEN_WRITE)
>                 bdev->bd_writers--;
> }

								Honza
 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR