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

[Darrick J. Wong]

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>

--D