This small series enables virtio-net device type in VDUSE.
With it, basic operation have been tested, both with
virtio-vdpa and vhost-vdpa using DPDK Vhost library series
adding VDUSE support using split rings layout (merged in
DPDK v23.07-rc1).
Control queue support (and so multiqueue) has also been
tested, but requires a Kernel series from Jason Wang
relaxing control queue polling [1] to function reliably,
so while Jason rework is done, a patch is added to disable
CVQ and features that depend on it (tested also with DPDK
v23.07-rc1).
In this v5, LSM hooks introduced in previous revision are
unified into a single hook that covers below operations:
- VDUSE_CREATE_DEV ioctl on VDUSE control file,
- VDUSE_DESTROY_DEV ioctl on VDUSE control file,
- open() on VDUSE device file.
In combination with the operations permission, a device type
permission has to be associated:
- block: Virtio block device type,
- net: Virtio networking device type.
Changes in v5:
==============
- Move control queue disablement patch before Net
devices enablement (Jason).
- Unify operations LSM hooks into a single hook.
- Rebase on latest master.
Maxime Coquelin (4):
vduse: validate block features only with block devices
vduse: Temporarily disable control queue features
vduse: enable Virtio-net device type
vduse: Add LSM hook to check Virtio device type
MAINTAINERS | 1 +
drivers/vdpa/vdpa_user/vduse_dev.c | 65 +++++++++++++++++++++++++++--
include/linux/lsm_hook_defs.h | 2 +
include/linux/security.h | 6 +++
include/linux/vduse.h | 14 +++++++
security/security.c | 15 +++++++
security/selinux/hooks.c | 32 ++++++++++++++
security/selinux/include/classmap.h | 2 +
8 files changed, 133 insertions(+), 4 deletions(-)
create mode 100644 include/linux/vduse.h
--
2.43.0
This patch adds Virtio-net device type to the supported
devices types. Initialization fails if the device does
not support VIRTIO_F_VERSION_1 feature, in order to
guarantee the configuration space is read-only.
Acked-by: Jason Wang <[email protected]>
Reviewed-by: Xie Yongji <[email protected]>
Signed-off-by: Maxime Coquelin <[email protected]>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index fe4b5c8203fd..fa62825be378 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -166,6 +166,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
static u32 allowed_device_id[] = {
VIRTIO_ID_BLOCK,
+ VIRTIO_ID_NET,
};
static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
@@ -1706,6 +1707,10 @@ static bool features_is_valid(struct vduse_dev_config *config)
(config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
+ if ((config->device_id == VIRTIO_ID_NET) &&
+ !(config->features & (1ULL << VIRTIO_F_VERSION_1)))
+ return false;
+
return true;
}
@@ -2068,6 +2073,7 @@ static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = {
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
+ { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
};
--
2.43.0
This patch introduces a LSM hook for devices creation,
destruction (ioctl()) and opening (open()) operations,
checking the application is allowed to perform these
operations for the Virtio device type.
Signed-off-by: Maxime Coquelin <[email protected]>
---
MAINTAINERS | 1 +
drivers/vdpa/vdpa_user/vduse_dev.c | 13 ++++++++++++
include/linux/lsm_hook_defs.h | 2 ++
include/linux/security.h | 6 ++++++
include/linux/vduse.h | 14 +++++++++++++
security/security.c | 15 ++++++++++++++
security/selinux/hooks.c | 32 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 ++
8 files changed, 85 insertions(+)
create mode 100644 include/linux/vduse.h
diff --git a/MAINTAINERS b/MAINTAINERS
index a0fb0df07b43..4e83b14358d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23040,6 +23040,7 @@ F: drivers/net/virtio_net.c
F: drivers/vdpa/
F: drivers/virtio/
F: include/linux/vdpa.h
+F: include/linux/vduse.h
F: include/linux/virtio*.h
F: include/linux/vringh.h
F: include/uapi/linux/virtio_*.h
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index fa62825be378..59ab7eb62e20 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -8,6 +8,7 @@
*
*/
+#include "linux/security.h"
#include <linux/init.h>
#include <linux/module.h>
#include <linux/cdev.h>
@@ -30,6 +31,7 @@
#include <uapi/linux/virtio_blk.h>
#include <uapi/linux/virtio_ring.h>
#include <linux/mod_devicetable.h>
+#include <linux/vduse.h>
#include "iova_domain.h"
@@ -1442,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct file *file)
if (dev->connected)
goto unlock;
+ ret = -EPERM;
+ if (security_vduse_perm_check(VDUSE_PERM_OPEN, dev->device_id))
+ goto unlock;
+
ret = 0;
dev->connected = true;
file->private_data = dev;
@@ -1664,6 +1670,9 @@ static int vduse_destroy_dev(char *name)
if (!dev)
return -EINVAL;
+ if (security_vduse_perm_check(VDUSE_PERM_DESTROY, dev->device_id))
+ return -EPERM;
+
mutex_lock(&dev->lock);
if (dev->vdev || dev->connected) {
mutex_unlock(&dev->lock);
@@ -1828,6 +1837,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
int ret;
struct vduse_dev *dev;
+ ret = -EPERM;
+ if (security_vduse_perm_check(VDUSE_PERM_CREATE, config->device_id))
+ goto err;
+
ret = -EEXIST;
if (vduse_find_dev(config->name))
goto err;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ff217a5ce552..3930ab2ae974 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -419,3 +419,5 @@ LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
LSM_HOOK(int, 0, uring_sqpoll, void)
LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
#endif /* CONFIG_IO_URING */
+
+LSM_HOOK(int, 0, vduse_perm_check, enum vduse_op_perm op_perm, u32 device_id)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1d1df326c881..2a2054172394 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -32,6 +32,7 @@
#include <linux/string.h>
#include <linux/mm.h>
#include <linux/sockptr.h>
+#include <linux/vduse.h>
struct linux_binprm;
struct cred;
@@ -484,6 +485,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
int security_locked_down(enum lockdown_reason what);
+int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id);
#else /* CONFIG_SECURITY */
static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1395,6 +1397,10 @@ static inline int security_locked_down(enum lockdown_reason what)
{
return 0;
}
+static inline int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
+{
+ return 0;
+}
#endif /* CONFIG_SECURITY */
#if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
diff --git a/include/linux/vduse.h b/include/linux/vduse.h
new file mode 100644
index 000000000000..7a20dcc43997
--- /dev/null
+++ b/include/linux/vduse.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_VDUSE_H
+#define _LINUX_VDUSE_H
+
+/*
+ * The permission required for a VDUSE device operation.
+ */
+enum vduse_op_perm {
+ VDUSE_PERM_CREATE,
+ VDUSE_PERM_DESTROY,
+ VDUSE_PERM_OPEN,
+};
+
+#endif /* _LINUX_VDUSE_H */
diff --git a/security/security.c b/security/security.c
index dcb3e7014f9b..150abf85f97d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5337,3 +5337,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
return call_int_hook(uring_cmd, 0, ioucmd);
}
#endif /* CONFIG_IO_URING */
+
+/**
+ * security_vduse_perm_check() - Check if a VDUSE device type operation is allowed
+ * @op_perm: the operation type
+ * @device_id: the Virtio device ID
+ *
+ * Check whether the Virtio device creation is allowed
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
+{
+ return call_int_hook(vduse_perm_check, 0, op_perm, device_id);
+}
+EXPORT_SYMBOL(security_vduse_perm_check);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index feda711c6b7b..18845e4f682f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -21,6 +21,8 @@
* Copyright (C) 2016 Mellanox Technologies
*/
+#include "av_permissions.h"
+#include "linux/vduse.h"
#include <linux/init.h>
#include <linux/kd.h>
#include <linux/kernel.h>
@@ -92,6 +94,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 +6953,34 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
}
#endif /* CONFIG_IO_URING */
+static int selinux_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
+{
+ u32 requested_op, requested_type, sid = current_sid();
+ int ret;
+
+ if (op_perm == VDUSE_PERM_CREATE)
+ requested_op = VDUSE__CREATE;
+ else if (op_perm == VDUSE__DESTROY)
+ requested_op = VDUSE__DESTROY;
+ else if (op_perm == VDUSE_PERM_OPEN)
+ requested_op = VDUSE__OPEN;
+ else
+ return -EINVAL;
+
+ ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_op, NULL);
+ if (ret)
+ return ret;
+
+ if (device_id == VIRTIO_ID_NET)
+ requested_type = VDUSE__NET;
+ else if (device_id == VIRTIO_ID_BLOCK)
+ requested_type = VDUSE__BLOCK;
+ else
+ return -EINVAL;
+
+ return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_type, NULL);
+}
+
/*
* 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 +7274,7 @@ 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_perm_check, selinux_vduse_perm_check),
};
static __init int selinux_init(void)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index a3c380775d41..b0a358cbac1c 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",
+ { "create", "destroy", "open", "net", "block", NULL} },
{ NULL }
};
--
2.43.0
This patch is preliminary work to enable network device
type support to VDUSE.
As VIRTIO_BLK_F_CONFIG_WCE shares the same value as
VIRTIO_NET_F_HOST_TSO4, we need to restrict its check
to Virtio-blk device type.
Acked-by: Jason Wang <[email protected]>
Reviewed-by: Xie Yongji <[email protected]>
Signed-off-by: Maxime Coquelin <[email protected]>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0ddd4b8abecb..0486ff672408 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1671,13 +1671,14 @@ static bool device_is_allowed(u32 device_id)
return false;
}
-static bool features_is_valid(u64 features)
+static bool features_is_valid(struct vduse_dev_config *config)
{
- if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
+ if (!(config->features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return false;
/* Now we only support read-only configuration space */
- if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
+ if ((config->device_id == VIRTIO_ID_BLOCK) &&
+ (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
return true;
@@ -1704,7 +1705,7 @@ static bool vduse_validate_config(struct vduse_dev_config *config)
if (!device_is_allowed(config->device_id))
return false;
- if (!features_is_valid(config->features))
+ if (!features_is_valid(config))
return false;
return true;
--
2.43.0
Virtio-net driver control queue implementation is not safe
when used with VDUSE. If the VDUSE application does not
reply to control queue messages, it currently ends up
hanging the kernel thread sending this command.
Some work is on-going to make the control queue
implementation robust with VDUSE. Until it is completed,
let's disable control virtqueue and features that depend on
it.
Signed-off-by: Maxime Coquelin <[email protected]>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0486ff672408..fe4b5c8203fd 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -28,6 +28,7 @@
#include <uapi/linux/virtio_config.h>
#include <uapi/linux/virtio_ids.h>
#include <uapi/linux/virtio_blk.h>
+#include <uapi/linux/virtio_ring.h>
#include <linux/mod_devicetable.h>
#include "iova_domain.h"
@@ -46,6 +47,30 @@
#define IRQ_UNBOUND -1
+#define VDUSE_NET_VALID_FEATURES_MASK \
+ (BIT_ULL(VIRTIO_NET_F_CSUM) | \
+ BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) | \
+ BIT_ULL(VIRTIO_NET_F_MTU) | \
+ BIT_ULL(VIRTIO_NET_F_MAC) | \
+ BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) | \
+ BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) | \
+ BIT_ULL(VIRTIO_NET_F_GUEST_ECN) | \
+ BIT_ULL(VIRTIO_NET_F_GUEST_UFO) | \
+ BIT_ULL(VIRTIO_NET_F_HOST_TSO4) | \
+ BIT_ULL(VIRTIO_NET_F_HOST_TSO6) | \
+ BIT_ULL(VIRTIO_NET_F_HOST_ECN) | \
+ BIT_ULL(VIRTIO_NET_F_HOST_UFO) | \
+ BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) | \
+ BIT_ULL(VIRTIO_NET_F_STATUS) | \
+ BIT_ULL(VIRTIO_NET_F_HOST_USO) | \
+ BIT_ULL(VIRTIO_F_ANY_LAYOUT) | \
+ BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
+ BIT_ULL(VIRTIO_RING_F_EVENT_IDX) | \
+ BIT_ULL(VIRTIO_F_VERSION_1) | \
+ BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
+ BIT_ULL(VIRTIO_F_RING_PACKED) | \
+ BIT_ULL(VIRTIO_F_IN_ORDER))
+
struct vduse_virtqueue {
u16 index;
u16 num_max;
@@ -1782,6 +1807,16 @@ static struct attribute *vduse_dev_attrs[] = {
ATTRIBUTE_GROUPS(vduse_dev);
+static void vduse_dev_features_filter(struct vduse_dev_config *config)
+{
+ /*
+ * Temporarily filter out virtio-net's control virtqueue and features
+ * that depend on it while CVQ is being made more robust for VDUSE.
+ */
+ if (config->device_id == VIRTIO_ID_NET)
+ config->features &= VDUSE_NET_VALID_FEATURES_MASK;
+}
+
static int vduse_create_dev(struct vduse_dev_config *config,
void *config_buf, u64 api_version)
{
@@ -1797,6 +1832,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
if (!dev)
goto err;
+ vduse_dev_features_filter(config);
+
dev->api_version = api_version;
dev->device_features = config->features;
dev->device_id = config->device_id;
--
2.43.0
On 12/12/2023 5:17 AM, Maxime Coquelin wrote:
> This patch introduces a LSM hook for devices creation,
> destruction (ioctl()) and opening (open()) operations,
> checking the application is allowed to perform these
> operations for the Virtio device type.
My earlier comments on a vduse specific LSM hook still hold.
I would much prefer to see a device permissions hook(s) that
are useful for devices in general. Not just vduse devices.
I know that there are already some very special purpose LSM
hooks, but the experience with maintaining them is why I don't
want more of them.
>
> Signed-off-by: Maxime Coquelin <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/vdpa/vdpa_user/vduse_dev.c | 13 ++++++++++++
> include/linux/lsm_hook_defs.h | 2 ++
> include/linux/security.h | 6 ++++++
> include/linux/vduse.h | 14 +++++++++++++
> security/security.c | 15 ++++++++++++++
> security/selinux/hooks.c | 32 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 ++
> 8 files changed, 85 insertions(+)
> create mode 100644 include/linux/vduse.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a0fb0df07b43..4e83b14358d2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23040,6 +23040,7 @@ F: drivers/net/virtio_net.c
> F: drivers/vdpa/
> F: drivers/virtio/
> F: include/linux/vdpa.h
> +F: include/linux/vduse.h
> F: include/linux/virtio*.h
> F: include/linux/vringh.h
> F: include/uapi/linux/virtio_*.h
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index fa62825be378..59ab7eb62e20 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -8,6 +8,7 @@
> *
> */
>
> +#include "linux/security.h"
> #include <linux/init.h>
> #include <linux/module.h>
> #include <linux/cdev.h>
> @@ -30,6 +31,7 @@
> #include <uapi/linux/virtio_blk.h>
> #include <uapi/linux/virtio_ring.h>
> #include <linux/mod_devicetable.h>
> +#include <linux/vduse.h>
>
> #include "iova_domain.h"
>
> @@ -1442,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct file *file)
> if (dev->connected)
> goto unlock;
>
> + ret = -EPERM;
> + if (security_vduse_perm_check(VDUSE_PERM_OPEN, dev->device_id))
> + goto unlock;
> +
> ret = 0;
> dev->connected = true;
> file->private_data = dev;
> @@ -1664,6 +1670,9 @@ static int vduse_destroy_dev(char *name)
> if (!dev)
> return -EINVAL;
>
> + if (security_vduse_perm_check(VDUSE_PERM_DESTROY, dev->device_id))
> + return -EPERM;
> +
> mutex_lock(&dev->lock);
> if (dev->vdev || dev->connected) {
> mutex_unlock(&dev->lock);
> @@ -1828,6 +1837,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> int ret;
> struct vduse_dev *dev;
>
> + ret = -EPERM;
> + if (security_vduse_perm_check(VDUSE_PERM_CREATE, config->device_id))
> + goto err;
> +
> ret = -EEXIST;
> if (vduse_find_dev(config->name))
> goto err;
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ff217a5ce552..3930ab2ae974 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -419,3 +419,5 @@ LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
> LSM_HOOK(int, 0, uring_sqpoll, void)
> LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
> #endif /* CONFIG_IO_URING */
> +
> +LSM_HOOK(int, 0, vduse_perm_check, enum vduse_op_perm op_perm, u32 device_id)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1d1df326c881..2a2054172394 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -32,6 +32,7 @@
> #include <linux/string.h>
> #include <linux/mm.h>
> #include <linux/sockptr.h>
> +#include <linux/vduse.h>
>
> struct linux_binprm;
> struct cred;
> @@ -484,6 +485,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> int security_locked_down(enum lockdown_reason what);
> +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id);
> #else /* CONFIG_SECURITY */
>
> static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> @@ -1395,6 +1397,10 @@ static inline int security_locked_down(enum lockdown_reason what)
> {
> return 0;
> }
> +static inline int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> +{
> + return 0;
> +}
> #endif /* CONFIG_SECURITY */
>
> #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
> diff --git a/include/linux/vduse.h b/include/linux/vduse.h
> new file mode 100644
> index 000000000000..7a20dcc43997
> --- /dev/null
> +++ b/include/linux/vduse.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_VDUSE_H
> +#define _LINUX_VDUSE_H
> +
> +/*
> + * The permission required for a VDUSE device operation.
> + */
> +enum vduse_op_perm {
> + VDUSE_PERM_CREATE,
> + VDUSE_PERM_DESTROY,
> + VDUSE_PERM_OPEN,
> +};
> +
> +#endif /* _LINUX_VDUSE_H */
> diff --git a/security/security.c b/security/security.c
> index dcb3e7014f9b..150abf85f97d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -5337,3 +5337,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
> return call_int_hook(uring_cmd, 0, ioucmd);
> }
> #endif /* CONFIG_IO_URING */
> +
> +/**
> + * security_vduse_perm_check() - Check if a VDUSE device type operation is allowed
> + * @op_perm: the operation type
> + * @device_id: the Virtio device ID
> + *
> + * Check whether the Virtio device creation is allowed
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> +{
> + return call_int_hook(vduse_perm_check, 0, op_perm, device_id);
> +}
> +EXPORT_SYMBOL(security_vduse_perm_check);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index feda711c6b7b..18845e4f682f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -21,6 +21,8 @@
> * Copyright (C) 2016 Mellanox Technologies
> */
>
> +#include "av_permissions.h"
> +#include "linux/vduse.h"
> #include <linux/init.h>
> #include <linux/kd.h>
> #include <linux/kernel.h>
> @@ -92,6 +94,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 +6953,34 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> }
> #endif /* CONFIG_IO_URING */
>
> +static int selinux_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> +{
> + u32 requested_op, requested_type, sid = current_sid();
> + int ret;
> +
> + if (op_perm == VDUSE_PERM_CREATE)
> + requested_op = VDUSE__CREATE;
> + else if (op_perm == VDUSE__DESTROY)
> + requested_op = VDUSE__DESTROY;
> + else if (op_perm == VDUSE_PERM_OPEN)
> + requested_op = VDUSE__OPEN;
> + else
> + return -EINVAL;
> +
> + ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_op, NULL);
> + if (ret)
> + return ret;
> +
> + if (device_id == VIRTIO_ID_NET)
> + requested_type = VDUSE__NET;
> + else if (device_id == VIRTIO_ID_BLOCK)
> + requested_type = VDUSE__BLOCK;
> + else
> + return -EINVAL;
> +
> + return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_type, NULL);
> +}
> +
> /*
> * 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 +7274,7 @@ 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_perm_check, selinux_vduse_perm_check),
> };
>
> static __init int selinux_init(void)
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index a3c380775d41..b0a358cbac1c 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",
> + { "create", "destroy", "open", "net", "block", NULL} },
> { NULL }
> };
>
On Tue, Dec 12, 2023 at 08:33:39AM -0800, Casey Schaufler wrote:
> On 12/12/2023 5:17 AM, Maxime Coquelin wrote:
> > This patch introduces a LSM hook for devices creation,
> > destruction (ioctl()) and opening (open()) operations,
> > checking the application is allowed to perform these
> > operations for the Virtio device type.
>
> My earlier comments on a vduse specific LSM hook still hold.
> I would much prefer to see a device permissions hook(s) that
> are useful for devices in general. Not just vduse devices.
> I know that there are already some very special purpose LSM
> hooks, but the experience with maintaining them is why I don't
> want more of them.
What exactly does this mean? Devices like tap etc? How do we
find them all though?
> >
> > Signed-off-by: Maxime Coquelin <[email protected]>
> > ---
> > MAINTAINERS | 1 +
> > drivers/vdpa/vdpa_user/vduse_dev.c | 13 ++++++++++++
> > include/linux/lsm_hook_defs.h | 2 ++
> > include/linux/security.h | 6 ++++++
> > include/linux/vduse.h | 14 +++++++++++++
> > security/security.c | 15 ++++++++++++++
> > security/selinux/hooks.c | 32 +++++++++++++++++++++++++++++
> > security/selinux/include/classmap.h | 2 ++
> > 8 files changed, 85 insertions(+)
> > create mode 100644 include/linux/vduse.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a0fb0df07b43..4e83b14358d2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -23040,6 +23040,7 @@ F: drivers/net/virtio_net.c
> > F: drivers/vdpa/
> > F: drivers/virtio/
> > F: include/linux/vdpa.h
> > +F: include/linux/vduse.h
> > F: include/linux/virtio*.h
> > F: include/linux/vringh.h
> > F: include/uapi/linux/virtio_*.h
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index fa62825be378..59ab7eb62e20 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -8,6 +8,7 @@
> > *
> > */
> >
> > +#include "linux/security.h"
> > #include <linux/init.h>
> > #include <linux/module.h>
> > #include <linux/cdev.h>
> > @@ -30,6 +31,7 @@
> > #include <uapi/linux/virtio_blk.h>
> > #include <uapi/linux/virtio_ring.h>
> > #include <linux/mod_devicetable.h>
> > +#include <linux/vduse.h>
> >
> > #include "iova_domain.h"
> >
> > @@ -1442,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct file *file)
> > if (dev->connected)
> > goto unlock;
> >
> > + ret = -EPERM;
> > + if (security_vduse_perm_check(VDUSE_PERM_OPEN, dev->device_id))
> > + goto unlock;
> > +
> > ret = 0;
> > dev->connected = true;
> > file->private_data = dev;
> > @@ -1664,6 +1670,9 @@ static int vduse_destroy_dev(char *name)
> > if (!dev)
> > return -EINVAL;
> >
> > + if (security_vduse_perm_check(VDUSE_PERM_DESTROY, dev->device_id))
> > + return -EPERM;
> > +
> > mutex_lock(&dev->lock);
> > if (dev->vdev || dev->connected) {
> > mutex_unlock(&dev->lock);
> > @@ -1828,6 +1837,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > int ret;
> > struct vduse_dev *dev;
> >
> > + ret = -EPERM;
> > + if (security_vduse_perm_check(VDUSE_PERM_CREATE, config->device_id))
> > + goto err;
> > +
> > ret = -EEXIST;
> > if (vduse_find_dev(config->name))
> > goto err;
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index ff217a5ce552..3930ab2ae974 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -419,3 +419,5 @@ LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
> > LSM_HOOK(int, 0, uring_sqpoll, void)
> > LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
> > #endif /* CONFIG_IO_URING */
> > +
> > +LSM_HOOK(int, 0, vduse_perm_check, enum vduse_op_perm op_perm, u32 device_id)
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 1d1df326c881..2a2054172394 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -32,6 +32,7 @@
> > #include <linux/string.h>
> > #include <linux/mm.h>
> > #include <linux/sockptr.h>
> > +#include <linux/vduse.h>
> >
> > struct linux_binprm;
> > struct cred;
> > @@ -484,6 +485,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> > int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> > int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> > int security_locked_down(enum lockdown_reason what);
> > +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id);
> > #else /* CONFIG_SECURITY */
> >
> > static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> > @@ -1395,6 +1397,10 @@ static inline int security_locked_down(enum lockdown_reason what)
> > {
> > return 0;
> > }
> > +static inline int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> > +{
> > + return 0;
> > +}
> > #endif /* CONFIG_SECURITY */
> >
> > #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
> > diff --git a/include/linux/vduse.h b/include/linux/vduse.h
> > new file mode 100644
> > index 000000000000..7a20dcc43997
> > --- /dev/null
> > +++ b/include/linux/vduse.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _LINUX_VDUSE_H
> > +#define _LINUX_VDUSE_H
> > +
> > +/*
> > + * The permission required for a VDUSE device operation.
> > + */
> > +enum vduse_op_perm {
> > + VDUSE_PERM_CREATE,
> > + VDUSE_PERM_DESTROY,
> > + VDUSE_PERM_OPEN,
> > +};
> > +
> > +#endif /* _LINUX_VDUSE_H */
> > diff --git a/security/security.c b/security/security.c
> > index dcb3e7014f9b..150abf85f97d 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -5337,3 +5337,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
> > return call_int_hook(uring_cmd, 0, ioucmd);
> > }
> > #endif /* CONFIG_IO_URING */
> > +
> > +/**
> > + * security_vduse_perm_check() - Check if a VDUSE device type operation is allowed
> > + * @op_perm: the operation type
> > + * @device_id: the Virtio device ID
> > + *
> > + * Check whether the Virtio device creation is allowed
> > + *
> > + * Return: Returns 0 if permission is granted.
> > + */
> > +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> > +{
> > + return call_int_hook(vduse_perm_check, 0, op_perm, device_id);
> > +}
> > +EXPORT_SYMBOL(security_vduse_perm_check);
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index feda711c6b7b..18845e4f682f 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -21,6 +21,8 @@
> > * Copyright (C) 2016 Mellanox Technologies
> > */
> >
> > +#include "av_permissions.h"
> > +#include "linux/vduse.h"
> > #include <linux/init.h>
> > #include <linux/kd.h>
> > #include <linux/kernel.h>
> > @@ -92,6 +94,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 +6953,34 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> > }
> > #endif /* CONFIG_IO_URING */
> >
> > +static int selinux_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> > +{
> > + u32 requested_op, requested_type, sid = current_sid();
> > + int ret;
> > +
> > + if (op_perm == VDUSE_PERM_CREATE)
> > + requested_op = VDUSE__CREATE;
> > + else if (op_perm == VDUSE__DESTROY)
> > + requested_op = VDUSE__DESTROY;
> > + else if (op_perm == VDUSE_PERM_OPEN)
> > + requested_op = VDUSE__OPEN;
> > + else
> > + return -EINVAL;
> > +
> > + ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_op, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + if (device_id == VIRTIO_ID_NET)
> > + requested_type = VDUSE__NET;
> > + else if (device_id == VIRTIO_ID_BLOCK)
> > + requested_type = VDUSE__BLOCK;
> > + else
> > + return -EINVAL;
> > +
> > + return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_type, NULL);
> > +}
> > +
> > /*
> > * 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 +7274,7 @@ 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_perm_check, selinux_vduse_perm_check),
> > };
> >
> > static __init int selinux_init(void)
> > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> > index a3c380775d41..b0a358cbac1c 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",
> > + { "create", "destroy", "open", "net", "block", NULL} },
> > { NULL }
> > };
> >
On 12/12/2023 9:59 AM, Michael S. Tsirkin wrote:
> On Tue, Dec 12, 2023 at 08:33:39AM -0800, Casey Schaufler wrote:
>> On 12/12/2023 5:17 AM, Maxime Coquelin wrote:
>>> This patch introduces a LSM hook for devices creation,
>>> destruction (ioctl()) and opening (open()) operations,
>>> checking the application is allowed to perform these
>>> operations for the Virtio device type.
>> My earlier comments on a vduse specific LSM hook still hold.
>> I would much prefer to see a device permissions hook(s) that
>> are useful for devices in general. Not just vduse devices.
>> I know that there are already some very special purpose LSM
>> hooks, but the experience with maintaining them is why I don't
>> want more of them.
> What exactly does this mean?
You have proposed an LSM hook that is only useful for vduse.
You want to implement a set of controls that only apply to vduse.
I can't help but think that if someone (i.e. you) wants to control
device creation for vduse that there could well be a use case for
control over device creation for some other set of devices. It is
quite possible that someone out there is desperately trying to
solve the same problem you have, but with a different device.
I have no desire to have to deal with
security_vduse_perm_check()
security_odddev_perm_check()
...
security_evendev_perm_check()
when we should be able to have
security_device_perm_check()
that can service them all.
> Devices like tap etc? How do we
> find them all though?
I'm not suggesting you find them all. I'm suggesting that you provide
an interface that someone could use if they wanted to. I think you
will be surprised how many will appear (with complaints about the
interface you propose, of course) if you implement a generally useful
LSM hook.
>
>>> Signed-off-by: Maxime Coquelin <[email protected]>
>>> ---
>>> MAINTAINERS | 1 +
>>> drivers/vdpa/vdpa_user/vduse_dev.c | 13 ++++++++++++
>>> include/linux/lsm_hook_defs.h | 2 ++
>>> include/linux/security.h | 6 ++++++
>>> include/linux/vduse.h | 14 +++++++++++++
>>> security/security.c | 15 ++++++++++++++
>>> security/selinux/hooks.c | 32 +++++++++++++++++++++++++++++
>>> security/selinux/include/classmap.h | 2 ++
>>> 8 files changed, 85 insertions(+)
>>> create mode 100644 include/linux/vduse.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index a0fb0df07b43..4e83b14358d2 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -23040,6 +23040,7 @@ F: drivers/net/virtio_net.c
>>> F: drivers/vdpa/
>>> F: drivers/virtio/
>>> F: include/linux/vdpa.h
>>> +F: include/linux/vduse.h
>>> F: include/linux/virtio*.h
>>> F: include/linux/vringh.h
>>> F: include/uapi/linux/virtio_*.h
>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> index fa62825be378..59ab7eb62e20 100644
>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> @@ -8,6 +8,7 @@
>>> *
>>> */
>>>
>>> +#include "linux/security.h"
>>> #include <linux/init.h>
>>> #include <linux/module.h>
>>> #include <linux/cdev.h>
>>> @@ -30,6 +31,7 @@
>>> #include <uapi/linux/virtio_blk.h>
>>> #include <uapi/linux/virtio_ring.h>
>>> #include <linux/mod_devicetable.h>
>>> +#include <linux/vduse.h>
>>>
>>> #include "iova_domain.h"
>>>
>>> @@ -1442,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct file *file)
>>> if (dev->connected)
>>> goto unlock;
>>>
>>> + ret = -EPERM;
>>> + if (security_vduse_perm_check(VDUSE_PERM_OPEN, dev->device_id))
>>> + goto unlock;
>>> +
>>> ret = 0;
>>> dev->connected = true;
>>> file->private_data = dev;
>>> @@ -1664,6 +1670,9 @@ static int vduse_destroy_dev(char *name)
>>> if (!dev)
>>> return -EINVAL;
>>>
>>> + if (security_vduse_perm_check(VDUSE_PERM_DESTROY, dev->device_id))
>>> + return -EPERM;
>>> +
>>> mutex_lock(&dev->lock);
>>> if (dev->vdev || dev->connected) {
>>> mutex_unlock(&dev->lock);
>>> @@ -1828,6 +1837,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>>> int ret;
>>> struct vduse_dev *dev;
>>>
>>> + ret = -EPERM;
>>> + if (security_vduse_perm_check(VDUSE_PERM_CREATE, config->device_id))
>>> + goto err;
>>> +
>>> ret = -EEXIST;
>>> if (vduse_find_dev(config->name))
>>> goto err;
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>> index ff217a5ce552..3930ab2ae974 100644
>>> --- a/include/linux/lsm_hook_defs.h
>>> +++ b/include/linux/lsm_hook_defs.h
>>> @@ -419,3 +419,5 @@ LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
>>> LSM_HOOK(int, 0, uring_sqpoll, void)
>>> LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
>>> #endif /* CONFIG_IO_URING */
>>> +
>>> +LSM_HOOK(int, 0, vduse_perm_check, enum vduse_op_perm op_perm, u32 device_id)
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 1d1df326c881..2a2054172394 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -32,6 +32,7 @@
>>> #include <linux/string.h>
>>> #include <linux/mm.h>
>>> #include <linux/sockptr.h>
>>> +#include <linux/vduse.h>
>>>
>>> struct linux_binprm;
>>> struct cred;
>>> @@ -484,6 +485,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
>>> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
>>> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
>>> int security_locked_down(enum lockdown_reason what);
>>> +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id);
>>> #else /* CONFIG_SECURITY */
>>>
>>> static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
>>> @@ -1395,6 +1397,10 @@ static inline int security_locked_down(enum lockdown_reason what)
>>> {
>>> return 0;
>>> }
>>> +static inline int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
>>> +{
>>> + return 0;
>>> +}
>>> #endif /* CONFIG_SECURITY */
>>>
>>> #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
>>> diff --git a/include/linux/vduse.h b/include/linux/vduse.h
>>> new file mode 100644
>>> index 000000000000..7a20dcc43997
>>> --- /dev/null
>>> +++ b/include/linux/vduse.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _LINUX_VDUSE_H
>>> +#define _LINUX_VDUSE_H
>>> +
>>> +/*
>>> + * The permission required for a VDUSE device operation.
>>> + */
>>> +enum vduse_op_perm {
>>> + VDUSE_PERM_CREATE,
>>> + VDUSE_PERM_DESTROY,
>>> + VDUSE_PERM_OPEN,
>>> +};
>>> +
>>> +#endif /* _LINUX_VDUSE_H */
>>> diff --git a/security/security.c b/security/security.c
>>> index dcb3e7014f9b..150abf85f97d 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -5337,3 +5337,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
>>> return call_int_hook(uring_cmd, 0, ioucmd);
>>> }
>>> #endif /* CONFIG_IO_URING */
>>> +
>>> +/**
>>> + * security_vduse_perm_check() - Check if a VDUSE device type operation is allowed
>>> + * @op_perm: the operation type
>>> + * @device_id: the Virtio device ID
>>> + *
>>> + * Check whether the Virtio device creation is allowed
>>> + *
>>> + * Return: Returns 0 if permission is granted.
>>> + */
>>> +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
>>> +{
>>> + return call_int_hook(vduse_perm_check, 0, op_perm, device_id);
>>> +}
>>> +EXPORT_SYMBOL(security_vduse_perm_check);
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index feda711c6b7b..18845e4f682f 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -21,6 +21,8 @@
>>> * Copyright (C) 2016 Mellanox Technologies
>>> */
>>>
>>> +#include "av_permissions.h"
>>> +#include "linux/vduse.h"
>>> #include <linux/init.h>
>>> #include <linux/kd.h>
>>> #include <linux/kernel.h>
>>> @@ -92,6 +94,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 +6953,34 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
>>> }
>>> #endif /* CONFIG_IO_URING */
>>>
>>> +static int selinux_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
>>> +{
>>> + u32 requested_op, requested_type, sid = current_sid();
>>> + int ret;
>>> +
>>> + if (op_perm == VDUSE_PERM_CREATE)
>>> + requested_op = VDUSE__CREATE;
>>> + else if (op_perm == VDUSE__DESTROY)
>>> + requested_op = VDUSE__DESTROY;
>>> + else if (op_perm == VDUSE_PERM_OPEN)
>>> + requested_op = VDUSE__OPEN;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_op, NULL);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (device_id == VIRTIO_ID_NET)
>>> + requested_type = VDUSE__NET;
>>> + else if (device_id == VIRTIO_ID_BLOCK)
>>> + requested_type = VDUSE__BLOCK;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_type, NULL);
>>> +}
>>> +
>>> /*
>>> * 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 +7274,7 @@ 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_perm_check, selinux_vduse_perm_check),
>>> };
>>>
>>> static __init int selinux_init(void)
>>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>>> index a3c380775d41..b0a358cbac1c 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",
>>> + { "create", "destroy", "open", "net", "block", NULL} },
>>> { NULL }
>>> };
>>>
>
On Tue, Dec 12, 2023 at 9:17 PM Maxime Coquelin
<[email protected]> wrote:
>
> Virtio-net driver control queue implementation is not safe
> when used with VDUSE. If the VDUSE application does not
> reply to control queue messages, it currently ends up
> hanging the kernel thread sending this command.
>
> Some work is on-going to make the control queue
> implementation robust with VDUSE. Until it is completed,
> let's disable control virtqueue and features that depend on
> it.
>
> Signed-off-by: Maxime Coquelin <[email protected]>
I wonder if it's better to fail instead of a mask as a start.
Thanks
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 0486ff672408..fe4b5c8203fd 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -28,6 +28,7 @@
> #include <uapi/linux/virtio_config.h>
> #include <uapi/linux/virtio_ids.h>
> #include <uapi/linux/virtio_blk.h>
> +#include <uapi/linux/virtio_ring.h>
> #include <linux/mod_devicetable.h>
>
> #include "iova_domain.h"
> @@ -46,6 +47,30 @@
>
> #define IRQ_UNBOUND -1
>
> +#define VDUSE_NET_VALID_FEATURES_MASK \
> + (BIT_ULL(VIRTIO_NET_F_CSUM) | \
> + BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) | \
> + BIT_ULL(VIRTIO_NET_F_MTU) | \
> + BIT_ULL(VIRTIO_NET_F_MAC) | \
> + BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) | \
> + BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) | \
> + BIT_ULL(VIRTIO_NET_F_GUEST_ECN) | \
> + BIT_ULL(VIRTIO_NET_F_GUEST_UFO) | \
> + BIT_ULL(VIRTIO_NET_F_HOST_TSO4) | \
> + BIT_ULL(VIRTIO_NET_F_HOST_TSO6) | \
> + BIT_ULL(VIRTIO_NET_F_HOST_ECN) | \
> + BIT_ULL(VIRTIO_NET_F_HOST_UFO) | \
> + BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) | \
> + BIT_ULL(VIRTIO_NET_F_STATUS) | \
> + BIT_ULL(VIRTIO_NET_F_HOST_USO) | \
> + BIT_ULL(VIRTIO_F_ANY_LAYOUT) | \
> + BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
> + BIT_ULL(VIRTIO_RING_F_EVENT_IDX) | \
> + BIT_ULL(VIRTIO_F_VERSION_1) | \
> + BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
> + BIT_ULL(VIRTIO_F_RING_PACKED) | \
> + BIT_ULL(VIRTIO_F_IN_ORDER))
> +
> struct vduse_virtqueue {
> u16 index;
> u16 num_max;
> @@ -1782,6 +1807,16 @@ static struct attribute *vduse_dev_attrs[] = {
>
> ATTRIBUTE_GROUPS(vduse_dev);
>
> +static void vduse_dev_features_filter(struct vduse_dev_config *config)
> +{
> + /*
> + * Temporarily filter out virtio-net's control virtqueue and features
> + * that depend on it while CVQ is being made more robust for VDUSE.
> + */
> + if (config->device_id == VIRTIO_ID_NET)
> + config->features &= VDUSE_NET_VALID_FEATURES_MASK;
> +}
> +
> static int vduse_create_dev(struct vduse_dev_config *config,
> void *config_buf, u64 api_version)
> {
> @@ -1797,6 +1832,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> if (!dev)
> goto err;
>
> + vduse_dev_features_filter(config);
> +
> dev->api_version = api_version;
> dev->device_features = config->features;
> dev->device_id = config->device_id;
> --
> 2.43.0
>
Hi Jason,
On 12/13/23 05:52, Jason Wang wrote:
> On Tue, Dec 12, 2023 at 9:17 PM Maxime Coquelin
> <[email protected]> wrote:
>>
>> Virtio-net driver control queue implementation is not safe
>> when used with VDUSE. If the VDUSE application does not
>> reply to control queue messages, it currently ends up
>> hanging the kernel thread sending this command.
>>
>> Some work is on-going to make the control queue
>> implementation robust with VDUSE. Until it is completed,
>> let's disable control virtqueue and features that depend on
>> it.
>>
>> Signed-off-by: Maxime Coquelin <[email protected]>
>
> I wonder if it's better to fail instead of a mask as a start.
I think it is better to use a mask and not fail, so that we can in the
future use a recent VDUSE application with an older kernel.
Why would it be better to fail than negotiating?
Thanks,
Maxime
> Thanks
>
>> ---
>> drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 0486ff672408..fe4b5c8203fd 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -28,6 +28,7 @@
>> #include <uapi/linux/virtio_config.h>
>> #include <uapi/linux/virtio_ids.h>
>> #include <uapi/linux/virtio_blk.h>
>> +#include <uapi/linux/virtio_ring.h>
>> #include <linux/mod_devicetable.h>
>>
>> #include "iova_domain.h"
>> @@ -46,6 +47,30 @@
>>
>> #define IRQ_UNBOUND -1
>>
>> +#define VDUSE_NET_VALID_FEATURES_MASK \
>> + (BIT_ULL(VIRTIO_NET_F_CSUM) | \
>> + BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) | \
>> + BIT_ULL(VIRTIO_NET_F_MTU) | \
>> + BIT_ULL(VIRTIO_NET_F_MAC) | \
>> + BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) | \
>> + BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) | \
>> + BIT_ULL(VIRTIO_NET_F_GUEST_ECN) | \
>> + BIT_ULL(VIRTIO_NET_F_GUEST_UFO) | \
>> + BIT_ULL(VIRTIO_NET_F_HOST_TSO4) | \
>> + BIT_ULL(VIRTIO_NET_F_HOST_TSO6) | \
>> + BIT_ULL(VIRTIO_NET_F_HOST_ECN) | \
>> + BIT_ULL(VIRTIO_NET_F_HOST_UFO) | \
>> + BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) | \
>> + BIT_ULL(VIRTIO_NET_F_STATUS) | \
>> + BIT_ULL(VIRTIO_NET_F_HOST_USO) | \
>> + BIT_ULL(VIRTIO_F_ANY_LAYOUT) | \
>> + BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
>> + BIT_ULL(VIRTIO_RING_F_EVENT_IDX) | \
>> + BIT_ULL(VIRTIO_F_VERSION_1) | \
>> + BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
>> + BIT_ULL(VIRTIO_F_RING_PACKED) | \
>> + BIT_ULL(VIRTIO_F_IN_ORDER))
>> +
>> struct vduse_virtqueue {
>> u16 index;
>> u16 num_max;
>> @@ -1782,6 +1807,16 @@ static struct attribute *vduse_dev_attrs[] = {
>>
>> ATTRIBUTE_GROUPS(vduse_dev);
>>
>> +static void vduse_dev_features_filter(struct vduse_dev_config *config)
>> +{
>> + /*
>> + * Temporarily filter out virtio-net's control virtqueue and features
>> + * that depend on it while CVQ is being made more robust for VDUSE.
>> + */
>> + if (config->device_id == VIRTIO_ID_NET)
>> + config->features &= VDUSE_NET_VALID_FEATURES_MASK;
>> +}
>> +
>> static int vduse_create_dev(struct vduse_dev_config *config,
>> void *config_buf, u64 api_version)
>> {
>> @@ -1797,6 +1832,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>> if (!dev)
>> goto err;
>>
>> + vduse_dev_features_filter(config);
>> +
>> dev->api_version = api_version;
>> dev->device_features = config->features;
>> dev->device_id = config->device_id;
>> --
>> 2.43.0
>>
>
On Tue, Dec 12, 2023 at 02:55:33PM -0800, Casey Schaufler wrote:
> On 12/12/2023 9:59 AM, Michael S. Tsirkin wrote:
> > On Tue, Dec 12, 2023 at 08:33:39AM -0800, Casey Schaufler wrote:
> >> On 12/12/2023 5:17 AM, Maxime Coquelin wrote:
> >>> This patch introduces a LSM hook for devices creation,
> >>> destruction (ioctl()) and opening (open()) operations,
> >>> checking the application is allowed to perform these
> >>> operations for the Virtio device type.
> >> My earlier comments on a vduse specific LSM hook still hold.
> >> I would much prefer to see a device permissions hook(s) that
> >> are useful for devices in general. Not just vduse devices.
> >> I know that there are already some very special purpose LSM
> >> hooks, but the experience with maintaining them is why I don't
> >> want more of them.
> > What exactly does this mean?
>
> You have proposed an LSM hook that is only useful for vduse.
> You want to implement a set of controls that only apply to vduse.
> I can't help but think that if someone (i.e. you) wants to control
> device creation for vduse that there could well be a use case for
> control over device creation for some other set of devices. It is
> quite possible that someone out there is desperately trying to
> solve the same problem you have, but with a different device.
>
> I have no desire to have to deal with
> security_vduse_perm_check()
> security_odddev_perm_check()
> ...
> security_evendev_perm_check()
>
> when we should be able to have
> security_device_perm_check()
>
> that can service them all.
>
>
> > Devices like tap etc? How do we
> > find them all though?
>
> I'm not suggesting you find them all. I'm suggesting that you provide
> an interface that someone could use if they wanted to. I think you
> will be surprised how many will appear (with complaints about the
> interface you propose, of course) if you implement a generally useful
> LSM hook.
Right now you have create, destroy, and open. Are you expecting to add
other perms? These sound generic enough that it definitely seems worth
doing as Casey suggests. On the other hand, if this could become a
gateway to lsm device access hooks basically becoming ioctl, we might
want to consider that.
> >>> Signed-off-by: Maxime Coquelin <[email protected]>
> >>> ---
> >>> MAINTAINERS | 1 +
> >>> drivers/vdpa/vdpa_user/vduse_dev.c | 13 ++++++++++++
> >>> include/linux/lsm_hook_defs.h | 2 ++
> >>> include/linux/security.h | 6 ++++++
> >>> include/linux/vduse.h | 14 +++++++++++++
> >>> security/security.c | 15 ++++++++++++++
> >>> security/selinux/hooks.c | 32 +++++++++++++++++++++++++++++
> >>> security/selinux/include/classmap.h | 2 ++
> >>> 8 files changed, 85 insertions(+)
> >>> create mode 100644 include/linux/vduse.h
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index a0fb0df07b43..4e83b14358d2 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -23040,6 +23040,7 @@ F: drivers/net/virtio_net.c
> >>> F: drivers/vdpa/
> >>> F: drivers/virtio/
> >>> F: include/linux/vdpa.h
> >>> +F: include/linux/vduse.h
> >>> F: include/linux/virtio*.h
> >>> F: include/linux/vringh.h
> >>> F: include/uapi/linux/virtio_*.h
> >>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> >>> index fa62825be378..59ab7eb62e20 100644
> >>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> >>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> >>> @@ -8,6 +8,7 @@
> >>> *
> >>> */
> >>>
> >>> +#include "linux/security.h"
> >>> #include <linux/init.h>
> >>> #include <linux/module.h>
> >>> #include <linux/cdev.h>
> >>> @@ -30,6 +31,7 @@
> >>> #include <uapi/linux/virtio_blk.h>
> >>> #include <uapi/linux/virtio_ring.h>
> >>> #include <linux/mod_devicetable.h>
> >>> +#include <linux/vduse.h>
> >>>
> >>> #include "iova_domain.h"
> >>>
> >>> @@ -1442,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct file *file)
> >>> if (dev->connected)
> >>> goto unlock;
> >>>
> >>> + ret = -EPERM;
> >>> + if (security_vduse_perm_check(VDUSE_PERM_OPEN, dev->device_id))
> >>> + goto unlock;
> >>> +
> >>> ret = 0;
> >>> dev->connected = true;
> >>> file->private_data = dev;
> >>> @@ -1664,6 +1670,9 @@ static int vduse_destroy_dev(char *name)
> >>> if (!dev)
> >>> return -EINVAL;
> >>>
> >>> + if (security_vduse_perm_check(VDUSE_PERM_DESTROY, dev->device_id))
> >>> + return -EPERM;
> >>> +
> >>> mutex_lock(&dev->lock);
> >>> if (dev->vdev || dev->connected) {
> >>> mutex_unlock(&dev->lock);
> >>> @@ -1828,6 +1837,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >>> int ret;
> >>> struct vduse_dev *dev;
> >>>
> >>> + ret = -EPERM;
> >>> + if (security_vduse_perm_check(VDUSE_PERM_CREATE, config->device_id))
> >>> + goto err;
> >>> +
> >>> ret = -EEXIST;
> >>> if (vduse_find_dev(config->name))
> >>> goto err;
> >>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> >>> index ff217a5ce552..3930ab2ae974 100644
> >>> --- a/include/linux/lsm_hook_defs.h
> >>> +++ b/include/linux/lsm_hook_defs.h
> >>> @@ -419,3 +419,5 @@ LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
> >>> LSM_HOOK(int, 0, uring_sqpoll, void)
> >>> LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
> >>> #endif /* CONFIG_IO_URING */
> >>> +
> >>> +LSM_HOOK(int, 0, vduse_perm_check, enum vduse_op_perm op_perm, u32 device_id)
> >>> diff --git a/include/linux/security.h b/include/linux/security.h
> >>> index 1d1df326c881..2a2054172394 100644
> >>> --- a/include/linux/security.h
> >>> +++ b/include/linux/security.h
> >>> @@ -32,6 +32,7 @@
> >>> #include <linux/string.h>
> >>> #include <linux/mm.h>
> >>> #include <linux/sockptr.h>
> >>> +#include <linux/vduse.h>
> >>>
> >>> struct linux_binprm;
> >>> struct cred;
> >>> @@ -484,6 +485,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> >>> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> >>> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> >>> int security_locked_down(enum lockdown_reason what);
> >>> +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id);
> >>> #else /* CONFIG_SECURITY */
> >>>
> >>> static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> >>> @@ -1395,6 +1397,10 @@ static inline int security_locked_down(enum lockdown_reason what)
> >>> {
> >>> return 0;
> >>> }
> >>> +static inline int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> >>> +{
> >>> + return 0;
> >>> +}
> >>> #endif /* CONFIG_SECURITY */
> >>>
> >>> #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
> >>> diff --git a/include/linux/vduse.h b/include/linux/vduse.h
> >>> new file mode 100644
> >>> index 000000000000..7a20dcc43997
> >>> --- /dev/null
> >>> +++ b/include/linux/vduse.h
> >>> @@ -0,0 +1,14 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>> +#ifndef _LINUX_VDUSE_H
> >>> +#define _LINUX_VDUSE_H
> >>> +
> >>> +/*
> >>> + * The permission required for a VDUSE device operation.
> >>> + */
> >>> +enum vduse_op_perm {
> >>> + VDUSE_PERM_CREATE,
> >>> + VDUSE_PERM_DESTROY,
> >>> + VDUSE_PERM_OPEN,
> >>> +};
> >>> +
> >>> +#endif /* _LINUX_VDUSE_H */
> >>> diff --git a/security/security.c b/security/security.c
> >>> index dcb3e7014f9b..150abf85f97d 100644
> >>> --- a/security/security.c
> >>> +++ b/security/security.c
> >>> @@ -5337,3 +5337,18 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
> >>> return call_int_hook(uring_cmd, 0, ioucmd);
> >>> }
> >>> #endif /* CONFIG_IO_URING */
> >>> +
> >>> +/**
> >>> + * security_vduse_perm_check() - Check if a VDUSE device type operation is allowed
> >>> + * @op_perm: the operation type
> >>> + * @device_id: the Virtio device ID
> >>> + *
> >>> + * Check whether the Virtio device creation is allowed
> >>> + *
> >>> + * Return: Returns 0 if permission is granted.
> >>> + */
> >>> +int security_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> >>> +{
> >>> + return call_int_hook(vduse_perm_check, 0, op_perm, device_id);
> >>> +}
> >>> +EXPORT_SYMBOL(security_vduse_perm_check);
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index feda711c6b7b..18845e4f682f 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -21,6 +21,8 @@
> >>> * Copyright (C) 2016 Mellanox Technologies
> >>> */
> >>>
> >>> +#include "av_permissions.h"
> >>> +#include "linux/vduse.h"
> >>> #include <linux/init.h>
> >>> #include <linux/kd.h>
> >>> #include <linux/kernel.h>
> >>> @@ -92,6 +94,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 +6953,34 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> >>> }
> >>> #endif /* CONFIG_IO_URING */
> >>>
> >>> +static int selinux_vduse_perm_check(enum vduse_op_perm op_perm, u32 device_id)
> >>> +{
> >>> + u32 requested_op, requested_type, sid = current_sid();
> >>> + int ret;
> >>> +
> >>> + if (op_perm == VDUSE_PERM_CREATE)
> >>> + requested_op = VDUSE__CREATE;
> >>> + else if (op_perm == VDUSE__DESTROY)
> >>> + requested_op = VDUSE__DESTROY;
> >>> + else if (op_perm == VDUSE_PERM_OPEN)
> >>> + requested_op = VDUSE__OPEN;
> >>> + else
> >>> + return -EINVAL;
> >>> +
> >>> + ret = avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_op, NULL);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + if (device_id == VIRTIO_ID_NET)
> >>> + requested_type = VDUSE__NET;
> >>> + else if (device_id == VIRTIO_ID_BLOCK)
> >>> + requested_type = VDUSE__BLOCK;
> >>> + else
> >>> + return -EINVAL;
> >>> +
> >>> + return avc_has_perm(sid, sid, SECCLASS_VDUSE, requested_type, NULL);
> >>> +}
> >>> +
> >>> /*
> >>> * 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 +7274,7 @@ 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_perm_check, selinux_vduse_perm_check),
> >>> };
> >>>
> >>> static __init int selinux_init(void)
> >>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> >>> index a3c380775d41..b0a358cbac1c 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",
> >>> + { "create", "destroy", "open", "net", "block", NULL} },
> >>> { NULL }
> >>> };
> >>>
> >
On Wed, Dec 13, 2023 at 7:23 PM Maxime Coquelin
<[email protected]> wrote:
>
> Hi Jason,
>
> On 12/13/23 05:52, Jason Wang wrote:
> > On Tue, Dec 12, 2023 at 9:17 PM Maxime Coquelin
> > <[email protected]> wrote:
> >>
> >> Virtio-net driver control queue implementation is not safe
> >> when used with VDUSE. If the VDUSE application does not
> >> reply to control queue messages, it currently ends up
> >> hanging the kernel thread sending this command.
> >>
> >> Some work is on-going to make the control queue
> >> implementation robust with VDUSE. Until it is completed,
> >> let's disable control virtqueue and features that depend on
> >> it.
> >>
> >> Signed-off-by: Maxime Coquelin <[email protected]>
> >
> > I wonder if it's better to fail instead of a mask as a start.
>
> I think it is better to use a mask and not fail, so that we can in the
> future use a recent VDUSE application with an older kernel.
It may confuse the userspace unless userspace can do post check after
CREATE_DEV.
And for blk we fail when WCE is set in feature_is_valid():
static bool features_is_valid(u64 features)
{
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return false;
/* Now we only support read-only configuration space */
if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
return false;
return true;
}
Thanks
>
> Why would it be better to fail than negotiating?
>
> Thanks,
> Maxime
>
On 12/18/23 03:50, Jason Wang wrote:
> On Wed, Dec 13, 2023 at 7:23 PM Maxime Coquelin
> <[email protected]> wrote:
>>
>> Hi Jason,
>>
>> On 12/13/23 05:52, Jason Wang wrote:
>>> On Tue, Dec 12, 2023 at 9:17 PM Maxime Coquelin
>>> <[email protected]> wrote:
>>>>
>>>> Virtio-net driver control queue implementation is not safe
>>>> when used with VDUSE. If the VDUSE application does not
>>>> reply to control queue messages, it currently ends up
>>>> hanging the kernel thread sending this command.
>>>>
>>>> Some work is on-going to make the control queue
>>>> implementation robust with VDUSE. Until it is completed,
>>>> let's disable control virtqueue and features that depend on
>>>> it.
>>>>
>>>> Signed-off-by: Maxime Coquelin <[email protected]>
>>>
>>> I wonder if it's better to fail instead of a mask as a start.
>>
>> I think it is better to use a mask and not fail, so that we can in the
>> future use a recent VDUSE application with an older kernel.
>
> It may confuse the userspace unless userspace can do post check after
> CREATE_DEV.
>
> And for blk we fail when WCE is set in feature_is_valid():
>
> static bool features_is_valid(u64 features)
> {
> if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> return false;
>
> /* Now we only support read-only configuration space */
> if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
> return false;
>
> return true;
> }
Ok, consistency with other devices types is indeed better.
But should I fail if any of the feature advertised by the application is
not listed by the VDUSE driver, or just fail if control queue is being
advertised by the application?
Thanks,
Maxime
> Thanks
>
>>
>> Why would it be better to fail than negotiating?
>>
>> Thanks,
>> Maxime
>>
>
On Tue, Dec 12, 2023 at 8:17 AM Maxime Coquelin
<[email protected]> wrote:
>
> This patch introduces a LSM hook for devices creation,
> destruction (ioctl()) and opening (open()) operations,
> checking the application is allowed to perform these
> operations for the Virtio device type.
Can you explain why the existing LSM hooks and SELinux implementation
are not sufficient? We already control the ability to open device
nodes via selinux_inode_permission() and selinux_file_open(), and can
support fine-grained per-cmd ioctl checking via selinux_file_ioctl().
And it should already be possible to label these nodes distinctly
through existing mechanisms (file_contexts if udev-created/labeled,
genfs_contexts if kernel-created). What exactly can't you do today
that this hook enables?
On Mon, Dec 18, 2023 at 12:21 PM Stephen Smalley
<[email protected]> wrote:
>
> On Tue, Dec 12, 2023 at 8:17 AM Maxime Coquelin
> <[email protected]> wrote:
> >
> > This patch introduces a LSM hook for devices creation,
> > destruction (ioctl()) and opening (open()) operations,
> > checking the application is allowed to perform these
> > operations for the Virtio device type.
>
> Can you explain why the existing LSM hooks and SELinux implementation
> are not sufficient? We already control the ability to open device
> nodes via selinux_inode_permission() and selinux_file_open(), and can
> support fine-grained per-cmd ioctl checking via selinux_file_ioctl().
> And it should already be possible to label these nodes distinctly
> through existing mechanisms (file_contexts if udev-created/labeled,
> genfs_contexts if kernel-created). What exactly can't you do today
> that this hook enables?
(added Ondrej to the distribution; IMHO we should swap him into
MAINTAINERS in place of Eric Paris since Eric has long-since moved on
from SELinux and Ondrej serves in that capacity these days)
Other items to consider:
- If vduse devices are created using anonymous inodes, then SELinux
grew a general facility for labeling and controlling the creation of
those via selinux_inode_init_security_anon().
- You can encode information about the device into its SELinux type
that then allows you to distinguish things like net vs block based on
the device's SELinux security context rather than encoding that in the
permission bits.
- If you truly need new LSM hooks (which you need to prove first),
then you should pass some usable information about the object in
question to allow SELinux to find a security context for it. Like an
inode associated with the device, for example.
On Mon, Dec 18, 2023 at 12:21 PM Stephen Smalley
<[email protected]> wrote:
> On Tue, Dec 12, 2023 at 8:17 AM Maxime Coquelin
> <[email protected]> wrote:
> > This patch introduces a LSM hook for devices creation,
> > destruction (ioctl()) and opening (open()) operations,
> > checking the application is allowed to perform these
> > operations for the Virtio device type.
>
> Can you explain why the existing LSM hooks and SELinux implementation
> are not sufficient? We already control the ability to open device
> nodes via selinux_inode_permission() and selinux_file_open(), and can
> support fine-grained per-cmd ioctl checking via selinux_file_ioctl().
> And it should already be possible to label these nodes distinctly
> through existing mechanisms (file_contexts if udev-created/labeled,
> genfs_contexts if kernel-created). What exactly can't you do today
> that this hook enables?
I asked something similar back in the v4 patchset to see if there was
some special labeling concerns that required the use of a dedicated
hook and from what I can see there are none.
> Other items to consider:
> - If vduse devices are created using anonymous inodes, then SELinux
> grew a general facility for labeling and controlling the creation of
> those via selinux_inode_init_security_anon().
For the vduse folks, the associated LSM API function is
security_inode_init_security_anon(); please don't call into SELinux
directly.
> - You can encode information about the device into its SELinux type
> that then allows you to distinguish things like net vs block based on
> the device's SELinux security context rather than encoding that in the
> permission bits.
> - If you truly need new LSM hooks (which you need to prove first),
> then you should pass some usable information about the object in
> question to allow SELinux to find a security context for it. Like an
> inode associated with the device, for example.
I agree with Stephen and I still remain skeptical that these hooks are
needed. Until I see a compelling case as to why the existing LSM
hooks are not sufficient I can't ACK these hooks.
--
paul-moore.com
On Mon, Dec 18, 2023 at 5:21 PM Maxime Coquelin
<[email protected]> wrote:
>
>
>
> On 12/18/23 03:50, Jason Wang wrote:
> > On Wed, Dec 13, 2023 at 7:23 PM Maxime Coquelin
> > <[email protected]> wrote:
> >>
> >> Hi Jason,
> >>
> >> On 12/13/23 05:52, Jason Wang wrote:
> >>> On Tue, Dec 12, 2023 at 9:17 PM Maxime Coquelin
> >>> <[email protected]> wrote:
> >>>>
> >>>> Virtio-net driver control queue implementation is not safe
> >>>> when used with VDUSE. If the VDUSE application does not
> >>>> reply to control queue messages, it currently ends up
> >>>> hanging the kernel thread sending this command.
> >>>>
> >>>> Some work is on-going to make the control queue
> >>>> implementation robust with VDUSE. Until it is completed,
> >>>> let's disable control virtqueue and features that depend on
> >>>> it.
> >>>>
> >>>> Signed-off-by: Maxime Coquelin <[email protected]>
> >>>
> >>> I wonder if it's better to fail instead of a mask as a start.
> >>
> >> I think it is better to use a mask and not fail, so that we can in the
> >> future use a recent VDUSE application with an older kernel.
> >
> > It may confuse the userspace unless userspace can do post check after
> > CREATE_DEV.
> >
> > And for blk we fail when WCE is set in feature_is_valid():
> >
> > static bool features_is_valid(u64 features)
> > {
> > if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> > return false;
> >
> > /* Now we only support read-only configuration space */
> > if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
> > return false;
> >
> > return true;
> > }
>
> Ok, consistency with other devices types is indeed better.
>
> But should I fail if any of the feature advertised by the application is
> not listed by the VDUSE driver, or just fail if control queue is being
> advertised by the application?
Maybe it's better to fail for any other of the features that depend on
the control vq.
Thanks
>
> Thanks,
> Maxime
>
> > Thanks
> >
> >>
> >> Why would it be better to fail than negotiating?
> >>
> >> Thanks,
> >> Maxime
> >>
> >
>
On 12/18/23 18:33, Stephen Smalley wrote:
> On Mon, Dec 18, 2023 at 12:21 PM Stephen Smalley
> <[email protected]> wrote:
>>
>> On Tue, Dec 12, 2023 at 8:17 AM Maxime Coquelin
>> <[email protected]> wrote:
>>>
>>> This patch introduces a LSM hook for devices creation,
>>> destruction (ioctl()) and opening (open()) operations,
>>> checking the application is allowed to perform these
>>> operations for the Virtio device type.
>>
>> Can you explain why the existing LSM hooks and SELinux implementation
>> are not sufficient? We already control the ability to open device
>> nodes via selinux_inode_permission() and selinux_file_open(), and can
>> support fine-grained per-cmd ioctl checking via selinux_file_ioctl().
>> And it should already be possible to label these nodes distinctly
>> through existing mechanisms (file_contexts if udev-created/labeled,
>> genfs_contexts if kernel-created). What exactly can't you do today
>> that this hook enables?
>
> (added Ondrej to the distribution; IMHO we should swap him into
> MAINTAINERS in place of Eric Paris since Eric has long-since moved on
> from SELinux and Ondrej serves in that capacity these days)
>
> Other items to consider:
> - If vduse devices are created using anonymous inodes, then SELinux
> grew a general facility for labeling and controlling the creation of
> those via selinux_inode_init_security_anon().
> - You can encode information about the device into its SELinux type
> that then allows you to distinguish things like net vs block based on
> the device's SELinux security context rather than encoding that in the
> permission bits.
Got it, that seems indeed more appropriate than using persmission bits
for the device type.
> - If you truly need new LSM hooks (which you need to prove first),
> then you should pass some usable information about the object in
> question to allow SELinux to find a security context for it. Like an
> inode associated with the device, for example.
Ok.
>
Thanks for the insights, I'll try and see if I can follow your
recommendations in a dedicated series.
Maxime