Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 16:13:24 -0400
From:   Mike Snitzer <snitzer@kernel.org>
To:     Daniil Lunev <dlunev@chromium.org>
Cc:     Alasdair Kergon <agk@redhat.com>,
        Brian Geffon <bgeffon@google.com>, dm-devel@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] dm: add message command to disallow device open
Message-ID: <YtB45Lte5UhlEE6y@redhat.com>
References: <20220704000225.345536-1-dlunev@chromium.org>
 <20220704100221.1.I15b3f7a84ba5a97fde9276648e391b54957103ff@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220704100221.1.I15b3f7a84ba5a97fde9276648e391b54957103ff@changeid>
Precedence: bulk

On Sun, Jul 03 2022 at  8:02P -0400,
Daniil Lunev <dlunev@chromium.org> wrote:

> A message can be passed to device mapper to prohibit open on a certain
> mapped device. This makes possible to disallow userspace access to
> raw swapped data if the system uses device mapper to encrypt it at rest.
> 
> Signed-off-by: Daniil Lunev <dlunev@chromium.org>

This commit header and patch make little sense to me.

If you're concerned about a normal (non-root) user having read access
to the swap device then disallow non-root user access permissions on
the swap device.

Why is an encrypted swap device any different than any other encrypted
device?

As is, this patch seems to be the wrong way to achieve your desired
result.  If you or someone else on the chromium team can better
defend/explain the need for this change please do so.

Thanks,
Mike


> ---
> 
>  drivers/md/dm-core.h          |  1 +
>  drivers/md/dm-ioctl.c         | 10 ++++++++++
>  drivers/md/dm.c               | 12 ++++++++++++
>  drivers/md/dm.h               | 10 ++++++++++
>  include/uapi/linux/dm-ioctl.h |  5 +++++
>  5 files changed, 38 insertions(+)
> 
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index 4277853c75351..37529b605b7c4 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -140,6 +140,7 @@ struct mapped_device {
>  #define DMF_SUSPENDED_INTERNALLY 7
>  #define DMF_POST_SUSPENDING 8
>  #define DMF_EMULATE_ZONE_APPEND 9
> +#define DMF_DISALLOW_OPEN 10
>  
>  void disable_discard(struct mapped_device *md);
>  void disable_write_zeroes(struct mapped_device *md);
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 87310fceb0d86..e35d560aa2ff3 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -815,6 +815,9 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
>  	if (dm_test_deferred_remove_flag(md))
>  		param->flags |= DM_DEFERRED_REMOVE;
>  
> +	if (dm_test_disallow_open_flag(md))
> +		param->flags |= DM_DISALLOWED_OPEN;
> +
>  	param->dev = huge_encode_dev(disk_devt(disk));
>  
>  	/*
> @@ -1656,6 +1659,13 @@ static int message_for_md(struct mapped_device *md, unsigned argc, char **argv,
>  		}
>  		return dm_cancel_deferred_remove(md);
>  	}
> +	if (!strcasecmp(argv[0], "@disallow_open")) {
> +		if (argc != 1) {
> +			DMERR("Invalid arguments for @disallow_open");
> +			return -EINVAL;
> +		}
> +		return dm_disallow_open(md);
> +	}
>  
>  	r = dm_stats_message(md, argc, argv, result, maxlen);
>  	if (r < 2)
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 82957bd460e89..3e53d1bd40f0c 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -327,6 +327,7 @@ static int dm_blk_open(struct block_device *bdev, fmode_t mode)
>  		goto out;
>  
>  	if (test_bit(DMF_FREEING, &md->flags) ||
> +	    test_bit(DMF_DISALLOW_OPEN, &md->flags) ||
>  	    dm_deleting_md(md)) {
>  		md = NULL;
>  		goto out;
> @@ -403,6 +404,12 @@ int dm_cancel_deferred_remove(struct mapped_device *md)
>  	return r;
>  }
>  
> +int dm_disallow_open(struct mapped_device *md)
> +{
> +	set_bit(DMF_DISALLOW_OPEN, &md->flags);
> +	return 0;
> +}
> +
>  static void do_deferred_remove(struct work_struct *w)
>  {
>  	dm_deferred_remove();
> @@ -2883,6 +2890,11 @@ int dm_test_deferred_remove_flag(struct mapped_device *md)
>  	return test_bit(DMF_DEFERRED_REMOVE, &md->flags);
>  }
>  
> +int dm_test_disallow_open_flag(struct mapped_device *md)
> +{
> +	return test_bit(DMF_DISALLOW_OPEN, &md->flags);
> +}
> +
>  int dm_suspended(struct dm_target *ti)
>  {
>  	return dm_suspended_md(ti->table->md);
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 9013dc1a7b002..da27f9dfe1413 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -163,6 +163,16 @@ int dm_test_deferred_remove_flag(struct mapped_device *md);
>   */
>  void dm_deferred_remove(void);
>  
> +/*
> + * Test if the device is openable.
> + */
> +int dm_test_disallow_open_flag(struct mapped_device *md);
> +
> +/*
> + * Prevent new open request on the device.
> + */
> +int dm_disallow_open(struct mapped_device *md);
> +
>  /*
>   * The device-mapper can be driven through one of two interfaces;
>   * ioctl or filesystem, depending which patch you have applied.
> diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
> index 2e9550fef90fa..3b4d12d09c005 100644
> --- a/include/uapi/linux/dm-ioctl.h
> +++ b/include/uapi/linux/dm-ioctl.h
> @@ -382,4 +382,9 @@ enum {
>   */
>  #define DM_IMA_MEASUREMENT_FLAG	(1 << 19) /* In */
>  
> +/*
> + * If set, the device can not be opened.
> + */
> +#define DM_DISALLOWED_OPEN	(1 << 20) /* Out */
> +
>  #endif				/* _LINUX_DM_IOCTL_H */
> -- 
> 2.31.0
>