Re: [PATCH] block: Deny writable memory mapping if block is read-only

From: Loic Poulain <loic.poulain@linaro.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: axboe@kernel.dk, linux-block@vger.kernel.org
Subject: Re: [PATCH] block: Deny writable memory mapping if block is read-only
Date: Wed, 10 May 2023 16:17:01 +0200	[thread overview]
Message-ID: <CAMZdPi99xo5FfSbUydvP4RNqWvDGcOCccckhdXs6S7paOa5W+w@mail.gmail.com> (raw)
In-Reply-To: <ZFub1U5f1qR5hOwX@infradead.org>

On Wed, 10 May 2023 at 15:27, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, May 10, 2023 at 09:42:23AM +0200, Loic Poulain wrote:
> > User should not be able to write block device if it is read-only at
> > block level (e.g force_ro attribute). This is ensured in the regular
> > fops write operation (blkdev_write_iter) but not when writing via
> > user mapping (mmap), allowing user to actually write a read-only
> > block device via a PROT_WRITE mapping.
> >
> > Example: This can lead to integrity issue of eMMC boot partition
> > (e.g mmcblk0boot0) which is read-only by default.
> >
> > To fix this issue, simply deny shared writable mapping if the block
> > is readonly.
> >
> > Note: Block remains writable if switch to read-only is performed
> > after the initial mapping, but this is expected behavior according
> > to commit a32e236eb93e ("Partially revert "block: fail op_is_write()
> > requests to read-only partitions"")'.
>
> We should not be able to every mmap something (shared-)writable if the
> file descriptor.
>
> And ... we don't failed writable opens for block devices ?!?

No, because the file itself is writable, but not the underlying block.
I agree, it would make more sense to simply deny the block open fops
instead... but it could be considered as uapi breakage as we may have
some existing applications opening the device RW, and simply
ignore/discard the sys write errors for ro devices... but if it's
acceptable let's do it. For sure, we could argue that making the mmap
failing is also a change in uapi behavior, but except reconsidering
a32e236eb93e which may be obsolete today, I don't see a better
solution to prevent unwanted writing.

>
> Something like this is what we would need, but I really need to look
> into the history of this whole thing a bit more:
>
>
> diff --git a/block/bdev.c b/block/bdev.c
> index 1795c7d4b99efa..6dd6045672b8bf 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -724,6 +724,11 @@ struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode, void *holder)
>                 return ERR_PTR(-ENXIO);
>         disk = bdev->bd_disk;
>
> +       if ((mode & FMODE_WRITE) && bdev_read_only(bdev)) {
> +               ret = -EACCES;
> +               goto put_blkdev;
> +       }
> +
>         if (mode & FMODE_EXCL) {
>                 ret = bd_prepare_to_claim(bdev, holder);
>                 if (ret)
> @@ -798,7 +803,6 @@ EXPORT_SYMBOL(blkdev_get_by_dev);
>  struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
>                                         void *holder)
>  {
> -       struct block_device *bdev;
>         dev_t dev;
>         int error;
>
> @@ -806,13 +810,7 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
>         if (error)
>                 return ERR_PTR(error);
>
> -       bdev = blkdev_get_by_dev(dev, mode, holder);
> -       if (!IS_ERR(bdev) && (mode & FMODE_WRITE) && bdev_read_only(bdev)) {
> -               blkdev_put(bdev, mode);
> -               return ERR_PTR(-EACCES);
> -       }
> -
> -       return bdev;
> +       return blkdev_get_by_dev(dev, mode, holder);
>  }
>  EXPORT_SYMBOL(blkdev_get_by_path);
>