Re: [PATCH v2] PM: hibernate: restrict writes to the resume device

[Rafael J. Wysocki]

On Tue, May 26, 2020 at 6:19 PM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
>
> On Mon, May 25, 2020 at 12:52:17PM +0200, Rafael J. Wysocki wrote:
> > On Tue, May 19, 2020 at 8:14 PM Domenico Andreoli
> > <domenico.andreoli@xxxxxxxxx> wrote:
> > >
> > > From: Domenico Andreoli <domenico.andreoli@xxxxxxxxx>
> > >
> > > Hibernation via snapshot device requires write permission to the swap
> > > block device, the one that more often (but not necessarily) is used to
> > > store the hibernation image.
> > >
> > > With this patch, such permissions are granted iff:
> > >
> > > 1) snapshot device config option is enabled
> > > 2) swap partition is used as resume device
> > >
> > > In other circumstances the swap device is not writable from userspace.
> > >
> > > In order to achieve this, every write attempt to a swap device is
> > > checked against the device configured as part of the uswsusp API [0]
> > > using a pointer to the inode struct in memory. If the swap device being
> > > written was not configured for resuming, the write request is denied.
> > >
> > > NOTE: this implementation works only for swap block devices, where the
> > > inode configured by swapon (which sets S_SWAPFILE) is the same used
> > > by SNAPSHOT_SET_SWAP_AREA.
> > >
> > > In case of swap file, SNAPSHOT_SET_SWAP_AREA indeed receives the inode
> > > of the block device containing the filesystem where the swap file is
> > > located (+ offset in it) which is never passed to swapon and then has
> > > not set S_SWAPFILE.
> > >
> > > As result, the swap file itself (as a file) has never an option to be
> > > written from userspace. Instead it remains writable if accessed directly
> > > from the containing block device, which is always writeable from root.
> > >
> > > [0] Documentation/power/userland-swsusp.rst
> > >
> > > v2:
> > >  - rename is_hibernate_snapshot_dev() to is_hibernate_resume_dev()
> > >  - fix description so to correctly refer to the resume device
> > >
> > > Signed-off-by: Domenico Andreoli <domenico.andreoli@xxxxxxxxx>
> > > Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> > > Cc: Pavel Machek <pavel@xxxxxx>
> > > Cc: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > Cc: Christoph Hellwig <hch@xxxxxx>
> > > Cc: viro@xxxxxxxxxxxxxxxxxx
> > > Cc: tytso@xxxxxxx
> > > Cc: len.brown@xxxxxxxxx
> > > Cc: linux-pm@xxxxxxxxxxxxxxx
> > > Cc: linux-mm@xxxxxxxxx
> > > Cc: linux-xfs@xxxxxxxxxxxxxxx
> > > Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > >
> > > ---
> > >  fs/block_dev.c          |    3 +--
> > >  include/linux/suspend.h |    6 ++++++
> > >  kernel/power/user.c     |   14 +++++++++++++-
> > >  3 files changed, 20 insertions(+), 3 deletions(-)
> > >
> > > Index: b/include/linux/suspend.h
> > > ===================================================================
> > > --- a/include/linux/suspend.h
> > > +++ b/include/linux/suspend.h
> > > @@ -466,6 +466,12 @@ static inline bool system_entering_hiber
> > >  static inline bool hibernation_available(void) { return false; }
> > >  #endif /* CONFIG_HIBERNATION */
> > >
> > > +#ifdef CONFIG_HIBERNATION_SNAPSHOT_DEV
> > > +int is_hibernate_resume_dev(const struct inode *);
> > > +#else
> > > +static inline int is_hibernate_resume_dev(const struct inode *i) { return 0; }
> > > +#endif
> > > +
> > >  /* Hibernation and suspend events */
> > >  #define PM_HIBERNATION_PREPARE 0x0001 /* Going to hibernate */
> > >  #define PM_POST_HIBERNATION    0x0002 /* Hibernation finished */
> > > Index: b/kernel/power/user.c
> > > ===================================================================
> > > --- a/kernel/power/user.c
> > > +++ b/kernel/power/user.c
> > > @@ -35,8 +35,14 @@ static struct snapshot_data {
> > >         bool ready;
> > >         bool platform_support;
> > >         bool free_bitmaps;
> > > +       struct inode *bd_inode;
> > >  } snapshot_state;
> > >
> > > +int is_hibernate_resume_dev(const struct inode *bd_inode)
> > > +{
> > > +       return hibernation_available() && snapshot_state.bd_inode == bd_inode;
> > > +}
> > > +
> > >  static int snapshot_open(struct inode *inode, struct file *filp)
> > >  {
> > >         struct snapshot_data *data;
> > > @@ -95,6 +101,7 @@ static int snapshot_open(struct inode *i
> > >         data->frozen = false;
> > >         data->ready = false;
> > >         data->platform_support = false;
> > > +       data->bd_inode = NULL;
> > >
> > >   Unlock:
> > >         unlock_system_sleep();
> > > @@ -110,6 +117,7 @@ static int snapshot_release(struct inode
> > >
> > >         swsusp_free();
> > >         data = filp->private_data;
> > > +       data->bd_inode = NULL;
> > >         free_all_swap_pages(data->swap);
> > >         if (data->frozen) {
> > >                 pm_restore_gfp_mask();
> > > @@ -202,6 +210,7 @@ struct compat_resume_swap_area {
> > >  static int snapshot_set_swap_area(struct snapshot_data *data,
> > >                 void __user *argp)
> > >  {
> > > +       struct block_device *bdev;
> > >         sector_t offset;
> > >         dev_t swdev;
> > >
> > > @@ -232,9 +241,12 @@ static int snapshot_set_swap_area(struct
> > >                 data->swap = -1;
> > >                 return -EINVAL;
> > >         }
> > > -       data->swap = swap_type_of(swdev, offset, NULL);
> > > +       data->swap = swap_type_of(swdev, offset, &bdev);
> > >         if (data->swap < 0)
> > >                 return -ENODEV;
> > > +
> > > +       data->bd_inode = bdev->bd_inode;
> > > +       bdput(bdev);
> > >         return 0;
> > >  }
> > >
> > > Index: b/fs/block_dev.c
> > > ===================================================================
> > > --- a/fs/block_dev.c
> > > +++ b/fs/block_dev.c
> > > @@ -2023,8 +2023,7 @@ ssize_t blkdev_write_iter(struct kiocb *
> > >         if (bdev_read_only(I_BDEV(bd_inode)))
> > >                 return -EPERM;
> > >
> > > -       /* uswsusp needs write permission to the swap */
> > > -       if (IS_SWAPFILE(bd_inode) && !hibernation_available())
> > > +       if (IS_SWAPFILE(bd_inode) && !is_hibernate_resume_dev(bd_inode))
> > >                 return -ETXTBSY;
> > >
> > >         if (!iov_iter_count(from))
> > >
> > > --
> >
> > The patch looks OK to me.
> >
> > Darrick, what do you think?
>
> Looks fine to me too.
>
> I kinda wonder how uswsusp prevents the bdev from being swapoff'd (or
> just plain disappearing) such that bd_inode will never point to a
> recycled inode, but I guess since we're only comparing pointer values
> it's not a big deal for this patch...
>
> Acked-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>

Thanks!

So the patch has been applied as 5.8 material.

Cheers!