Re: [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type

[Paul Moore <paul@paul-moore.com>]

On Oct 20, 2023 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> This patch introduces LSM hooks for devices creation,
> destruction and opening operations, checking the
> application is allowed to perform these operations for
> the Virtio device type.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c  | 12 +++++++
>  include/linux/lsm_hook_defs.h       |  4 +++
>  include/linux/security.h            | 15 ++++++++
>  security/security.c                 | 42 ++++++++++++++++++++++
>  security/selinux/hooks.c            | 55 +++++++++++++++++++++++++++++
>  security/selinux/include/classmap.h |  2 ++
>  6 files changed, 130 insertions(+)

My apologies for the late reply, I've been trying to work my way through
the review backlog but it has been taking longer than expected; comments
below ...

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2aa0e219d721..65d9262a37f7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -21,6 +21,7 @@
>   *  Copyright (C) 2016 Mellanox Technologies
>   */
>  
> +#include "av_permissions.h"
>  #include <linux/init.h>
>  #include <linux/kd.h>
>  #include <linux/kernel.h>
> @@ -92,6 +93,7 @@
>  #include <linux/fsnotify.h>
>  #include <linux/fanotify.h>
>  #include <linux/io_uring.h>
> +#include <uapi/linux/virtio_ids.h>
>  
>  #include "avc.h"
>  #include "objsec.h"
> @@ -6950,6 +6952,56 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
>  }
>  #endif /* CONFIG_IO_URING */
>  
> +static int vduse_check_device_type(u32 sid, u32 device_id)
> +{
> +	u32 requested;
> +
> +	if (device_id == VIRTIO_ID_NET)
> +		requested = VDUSE__NET;
> +	else if (device_id == VIRTIO_ID_BLOCK)
> +		requested = VDUSE__BLOCK;
> +	else
> +		return -EINVAL;
> +
> +	return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested, NULL);
> +}
> +
> +static int selinux_vduse_dev_create(u32 device_id)
> +{
> +	u32 sid = current_sid();
> +	int ret;
> +
> +	ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVCREATE, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return vduse_check_device_type(sid, device_id);
> +}

I see there has been some discussion about the need for a dedicated
create hook as opposed to using the existing ioctl controls.  I think
one important point that has been missing from the discussion is the
idea of labeling the newly created device.  Unfortunately prior to a
few minutes ago I hadn't ever looked at VDUSE so please correct me if
I get some things wrong :)

From what I can see userspace creates a new VDUSE device with
ioctl(VDUSE_CREATE_DEV), which trigger the creation of a new
/dev/vduse/XXX device which will be labeled according to the udev
and SELinux configuration, likely with a generic udev label.  My
question is if we want to be able to uniquely label each VDUSE
device based on the process that initiates the device creation
with the call to ioctl()?  If that is the case, we would need a
create hook not only to control the creation of the device, but to
record the triggering process' label in the new device; this label
would then be used in subsequent VDUSE open and destroy operations.
The normal device file I/O operations would still be subject to the
standard SELinux file I/O permissions using the device file label
assigned by systemd/udev when the device was created.

> +static int selinux_vduse_dev_destroy(u32 device_id)
> +{
> +	u32 sid = current_sid();
> +	int ret;
> +
> +	ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVDESTROY, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return vduse_check_device_type(sid, device_id);
> +}
> +
> +static int selinux_vduse_dev_open(u32 device_id)
> +{
> +	u32 sid = current_sid();
> +	int ret;
> +
> +	ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, VDUSE__DEVOPEN, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return vduse_check_device_type(sid, device_id);
> +}
> +
>  /*
>   * IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
>   * 1. any hooks that don't belong to (2.) or (3.) below,
> @@ -7243,6 +7295,9 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>  #ifdef CONFIG_PERF_EVENTS
>  	LSM_HOOK_INIT(perf_event_alloc, selinux_perf_event_alloc),
>  #endif
> +	LSM_HOOK_INIT(vduse_dev_create, selinux_vduse_dev_create),
> +	LSM_HOOK_INIT(vduse_dev_destroy, selinux_vduse_dev_destroy),
> +	LSM_HOOK_INIT(vduse_dev_open, selinux_vduse_dev_open),
>  };
>  
>  static __init int selinux_init(void)
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index a3c380775d41..d3dc37fb03d4 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -256,6 +256,8 @@ const struct security_class_mapping secclass_map[] = {
>  	  { "override_creds", "sqpoll", "cmd", NULL } },
>  	{ "user_namespace",
>  	  { "create", NULL } },
> +	{ "vduse",
> +	  { "devcreate", "devdestroy", "devopen", "net", "block", NULL} },

I think we can just call the permissions "create", "open", and "destroy"
since the "dev" prefix is somewhat implied by this being a dedicated
VDUSE object class.

I don't see where you are using the "net" and "block" permissions above,
is this a leftover from a prior draft of this patch or are you planning
to do something with these permissions?

>  	{ NULL }
>    };
>  
> -- 
> 2.41.0

--
paul-moore.com