| Message ID | 20200519181410.GA1963@dumbo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] PM: hibernate: restrict writes to the resume device | expand |
On Tue, May 19, 2020 at 8:14 PM Domenico Andreoli <domenico.andreoli@linux.com> wrote: > > From: Domenico Andreoli <domenico.andreoli@linux.com> > > 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@linux.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Darrick J. Wong <darrick.wong@oracle.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: viro@zeniv.linux.org.uk > Cc: tytso@mit.edu > Cc: len.brown@intel.com > Cc: linux-pm@vger.kernel.org > Cc: linux-mm@kvack.org > Cc: linux-xfs@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > --- > 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?
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@linux.com> wrote: > > > > From: Domenico Andreoli <domenico.andreoli@linux.com> > > > > 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@linux.com> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > Cc: Pavel Machek <pavel@ucw.cz> > > Cc: Darrick J. Wong <darrick.wong@oracle.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: viro@zeniv.linux.org.uk > > Cc: tytso@mit.edu > > Cc: len.brown@intel.com > > Cc: linux-pm@vger.kernel.org > > Cc: linux-mm@kvack.org > > Cc: linux-xfs@vger.kernel.org > > Cc: linux-fsdevel@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > > > --- > > 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@oracle.com> --D
On Tue, May 26, 2020 at 6:19 PM Darrick J. Wong <darrick.wong@oracle.com> 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@linux.com> wrote: > > > > > > From: Domenico Andreoli <domenico.andreoli@linux.com> > > > > > > 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@linux.com> > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > Cc: Pavel Machek <pavel@ucw.cz> > > > Cc: Darrick J. Wong <darrick.wong@oracle.com> > > > Cc: Christoph Hellwig <hch@lst.de> > > > Cc: viro@zeniv.linux.org.uk > > > Cc: tytso@mit.edu > > > Cc: len.brown@intel.com > > > Cc: linux-pm@vger.kernel.org > > > Cc: linux-mm@kvack.org > > > Cc: linux-xfs@vger.kernel.org > > > Cc: linux-fsdevel@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org > > > > > > --- > > > 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@oracle.com> Thanks! So the patch has been applied as 5.8 material. Cheers!
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))