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 v4, LSM hooks are added to allow/deny application
to create/destroy/open devices based on their type (Net,
Block).
[1]: https://lore.kernel.org/lkml/CACGkMEtgrxN3PPwsDo4oOsnsSLJfEmBEZ0WvjGRr3whU+QasUg@mail.gmail.com/T/
v3->v4 changes:
===============
- Add LSM hooks (Michael)
- Rebase
v2 -> v3 changes:
=================
- Use allow list instead of deny list (Michael)
v1 -> v2 changes:
=================
- Add a patch to disable CVQ (Michael)
RFC -> v1 changes:
==================
- Fail device init if it does not support VERSION_1 (Jason)
Maxime Coquelin (4):
vduse: validate block features only with block devices
vduse: enable Virtio-net device type
vduse: Temporarily disable control queue features
vduse: Add LSM hooks to check Virtio device type
drivers/vdpa/vdpa_user/vduse_dev.c | 64 +++++++++++++++++++++++++++--
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, 178 insertions(+), 4 deletions(-)
--
2.41.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 5b3879976b3d..73ad3b7efd8e 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -142,6 +142,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)
@@ -1672,6 +1673,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;
}
@@ -2027,6 +2032,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.41.0
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 <[email protected]>
---
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(+)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0243dee9cf0e..ca64eac11ddb 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>
@@ -1443,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct file *file)
if (dev->connected)
goto unlock;
+ ret = -EPERM;
+ if (security_vduse_dev_open(dev->device_id))
+ goto unlock;
+
ret = 0;
dev->connected = true;
file->private_data = dev;
@@ -1655,6 +1660,9 @@ static int vduse_destroy_dev(char *name)
if (!dev)
return -EINVAL;
+ if (security_vduse_dev_destroy(dev->device_id))
+ return -EPERM;
+
mutex_lock(&dev->lock);
if (dev->vdev || dev->connected) {
mutex_unlock(&dev->lock);
@@ -1819,6 +1827,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
int ret;
struct vduse_dev *dev;
+ ret = -EPERM;
+ if (security_vduse_dev_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 ac962c4cb44b..0b3999ab3264 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -419,3 +419,7 @@ 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_dev_create, u32 device_id)
+LSM_HOOK(int, 0, vduse_dev_destroy, u32 device_id)
+LSM_HOOK(int, 0, vduse_dev_open, u32 device_id)
diff --git a/include/linux/security.h b/include/linux/security.h
index 5f16eecde00b..a650c500f841 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -484,6 +484,9 @@ 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_dev_create(u32 device_id);
+int security_vduse_dev_destroy(u32 device_id);
+int security_vduse_dev_open(u32 device_id);
#else /* CONFIG_SECURITY */
static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1395,6 +1398,18 @@ static inline int security_locked_down(enum lockdown_reason what)
{
return 0;
}
+static inline int security_vduse_dev_create(u32 device_id)
+{
+ return 0;
+}
+static inline int security_vduse_dev_destroy(u32 device_id)
+{
+ return 0;
+}
+static inline int security_vduse_dev_open(u32 device_id)
+{
+ return 0;
+}
#endif /* CONFIG_SECURITY */
#if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
diff --git a/security/security.c b/security/security.c
index 23b129d482a7..8d7d4d2eca0b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5337,3 +5337,45 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
return call_int_hook(uring_cmd, 0, ioucmd);
}
#endif /* CONFIG_IO_URING */
+
+/**
+ * security_vduse_dev_create() - Check if a VDUSE device type creation is allowed
+ * @device_id: the Virtio device ID
+ *
+ * Check whether the Virtio device creation is allowed
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_vduse_dev_create(u32 device_id)
+{
+ return call_int_hook(vduse_dev_create, 0, device_id);
+}
+EXPORT_SYMBOL(security_vduse_dev_create);
+
+/**
+ * security_vduse_dev_destroy() - Check if a VDUSE device type destruction is allowed
+ * @device_id: the Virtio device ID
+ *
+ * Check whether the Virtio device destruction is allowed
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_vduse_dev_destroy(u32 device_id)
+{
+ return call_int_hook(vduse_dev_destroy, 0, device_id);
+}
+EXPORT_SYMBOL(security_vduse_dev_destroy);
+
+/**
+ * security_vduse_dev_open() - Check if a VDUSE device type opening is allowed
+ * @device_id: the Virtio device ID
+ *
+ * Check whether the Virtio device opening is allowed
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_vduse_dev_open(u32 device_id)
+{
+ return call_int_hook(vduse_dev_open, 0, device_id);
+}
+EXPORT_SYMBOL(security_vduse_dev_open);
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);
+}
+
+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} },
{ NULL }
};
--
2.41.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 df7869537ef1..5b3879976b3d 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1662,13 +1662,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;
@@ -1695,7 +1696,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.41.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 73ad3b7efd8e..0243dee9cf0e 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;
@@ -1778,6 +1803,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)
{
@@ -1793,6 +1828,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.41.0
On 10/20/2023 8:58 AM, Maxime Coquelin wrote:
> 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 df7869537ef1..5b3879976b3d 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1662,13 +1662,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)
This should either be features_are_valid() or feature_is_valid().
Correct pluralization is important in the English language.
> {
> - 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;
> @@ -1695,7 +1696,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;
On 10/20/2023 8:58 AM, Maxime Coquelin 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.
Why do you think that there needs to be a special LSM check for virtio
devices? What can't existing device attributes be used?
>
> Signed-off-by: Maxime Coquelin <[email protected]>
> ---
> 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(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 0243dee9cf0e..ca64eac11ddb 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>
> @@ -1443,6 +1444,10 @@ static int vduse_dev_open(struct inode *inode, struct file *file)
> if (dev->connected)
> goto unlock;
>
> + ret = -EPERM;
> + if (security_vduse_dev_open(dev->device_id))
> + goto unlock;
> +
> ret = 0;
> dev->connected = true;
> file->private_data = dev;
> @@ -1655,6 +1660,9 @@ static int vduse_destroy_dev(char *name)
> if (!dev)
> return -EINVAL;
>
> + if (security_vduse_dev_destroy(dev->device_id))
> + return -EPERM;
> +
> mutex_lock(&dev->lock);
> if (dev->vdev || dev->connected) {
> mutex_unlock(&dev->lock);
> @@ -1819,6 +1827,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> int ret;
> struct vduse_dev *dev;
>
> + ret = -EPERM;
> + if (security_vduse_dev_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 ac962c4cb44b..0b3999ab3264 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -419,3 +419,7 @@ 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_dev_create, u32 device_id)
> +LSM_HOOK(int, 0, vduse_dev_destroy, u32 device_id)
> +LSM_HOOK(int, 0, vduse_dev_open, u32 device_id)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5f16eecde00b..a650c500f841 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -484,6 +484,9 @@ 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_dev_create(u32 device_id);
> +int security_vduse_dev_destroy(u32 device_id);
> +int security_vduse_dev_open(u32 device_id);
> #else /* CONFIG_SECURITY */
>
> static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> @@ -1395,6 +1398,18 @@ static inline int security_locked_down(enum lockdown_reason what)
> {
> return 0;
> }
> +static inline int security_vduse_dev_create(u32 device_id)
> +{
> + return 0;
> +}
> +static inline int security_vduse_dev_destroy(u32 device_id)
> +{
> + return 0;
> +}
> +static inline int security_vduse_dev_open(u32 device_id)
> +{
> + return 0;
> +}
> #endif /* CONFIG_SECURITY */
>
> #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
> diff --git a/security/security.c b/security/security.c
> index 23b129d482a7..8d7d4d2eca0b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -5337,3 +5337,45 @@ int security_uring_cmd(struct io_uring_cmd *ioucmd)
> return call_int_hook(uring_cmd, 0, ioucmd);
> }
> #endif /* CONFIG_IO_URING */
> +
> +/**
> + * security_vduse_dev_create() - Check if a VDUSE device type creation is allowed
> + * @device_id: the Virtio device ID
> + *
> + * Check whether the Virtio device creation is allowed
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_vduse_dev_create(u32 device_id)
> +{
> + return call_int_hook(vduse_dev_create, 0, device_id);
> +}
> +EXPORT_SYMBOL(security_vduse_dev_create);
> +
> +/**
> + * security_vduse_dev_destroy() - Check if a VDUSE device type destruction is allowed
> + * @device_id: the Virtio device ID
> + *
> + * Check whether the Virtio device destruction is allowed
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_vduse_dev_destroy(u32 device_id)
> +{
> + return call_int_hook(vduse_dev_destroy, 0, device_id);
> +}
> +EXPORT_SYMBOL(security_vduse_dev_destroy);
> +
> +/**
> + * security_vduse_dev_open() - Check if a VDUSE device type opening is allowed
> + * @device_id: the Virtio device ID
> + *
> + * Check whether the Virtio device opening is allowed
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_vduse_dev_open(u32 device_id)
> +{
> + return call_int_hook(vduse_dev_open, 0, device_id);
> +}
> +EXPORT_SYMBOL(security_vduse_dev_open);
> 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);
> +}
> +
> +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} },
> { NULL }
> };
>
On Fri, Oct 20, 2023 at 11:58 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 do this with patch 2 or before patch 2 to
unbreak the bisection?
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 73ad3b7efd8e..0243dee9cf0e 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;
> @@ -1778,6 +1803,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)
> {
> @@ -1793,6 +1828,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.41.0
>
On Fri, Oct 20, 2023 at 11:58 PM Maxime Coquelin
<[email protected]> 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 <[email protected]>
> ---
Hi Maxime:
I think we need to document the reason why we need those hooks now.
Thanks
On 10/21/23 00:20, Casey Schaufler wrote:
> On 10/20/2023 8:58 AM, Maxime Coquelin 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.
>
> Why do you think that there needs to be a special LSM check for virtio
> devices? What can't existing device attributes be used?
Michael asked for a way for SELinux to allow/prevent the creation of
some types of devices [0].
A device is created using ioctl() on VDUSE control chardev. Its type is
specified via a field in the structure passed in argument.
I didn't see other way than adding dedicated LSM hooks to achieve this,
but it is possible that their is a better way to do it?
Thanks,
Maxime
[0]:
https://lore.kernel.org/all/[email protected]/
On 10/21/23 00:07, Casey Schaufler wrote:
> On 10/20/2023 8:58 AM, Maxime Coquelin wrote:
>> 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 df7869537ef1..5b3879976b3d 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -1662,13 +1662,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)
>
> This should either be features_are_valid() or feature_is_valid().
> Correct pluralization is important in the English language.
Indeed, I will change to features_are_valid() in next revision.
Thanks,
Maxime
>> {
>> - 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;
>> @@ -1695,7 +1696,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;
>
On 10/23/23 05:08, Jason Wang wrote:
> On Fri, Oct 20, 2023 at 11:58 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 do this with patch 2 or before patch 2 to
> unbreak the bisection?
I think it would be better to keep it in a dedicated patch to ease the
revert later when your work will have been accepted, so before patch 2.
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 73ad3b7efd8e..0243dee9cf0e 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;
>> @@ -1778,6 +1803,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)
>> {
>> @@ -1793,6 +1828,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.41.0
>>
>
On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
>
>
> On 10/21/23 00:20, Casey Schaufler wrote:
>> On 10/20/2023 8:58 AM, Maxime Coquelin 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.
>>
>> Why do you think that there needs to be a special LSM check for virtio
>> devices? What can't existing device attributes be used?
>
> Michael asked for a way for SELinux to allow/prevent the creation of
> some types of devices [0].
>
> A device is created using ioctl() on VDUSE control chardev. Its type is
> specified via a field in the structure passed in argument.
>
> I didn't see other way than adding dedicated LSM hooks to achieve this,
> but it is possible that their is a better way to do it?
At the very least the hook should be made more general, and I'd have to
see a proposal before commenting on that. security_dev_destroy(dev) might
be a better approach. If there's reason to control destruction of vduse
devices it's reasonable to assume that there are other devices with the
same or similar properties.
Since SELinux is your target use case, can you explain why you can't
create SELinux policy to enforce the restrictions you're after? I believe
(but can be proven wrong, of course) that SELinux has mechanism for dealing
with controls on ioctls.
>
> Thanks,
> Maxime
>
> [0]:
> https://lore.kernel.org/all/[email protected]/
>
On 10/23/23 17:13, Casey Schaufler wrote:
> On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
>>
>>
>> On 10/21/23 00:20, Casey Schaufler wrote:
>>> On 10/20/2023 8:58 AM, Maxime Coquelin 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.
>>>
>>> Why do you think that there needs to be a special LSM check for virtio
>>> devices? What can't existing device attributes be used?
>>
>> Michael asked for a way for SELinux to allow/prevent the creation of
>> some types of devices [0].
>>
>> A device is created using ioctl() on VDUSE control chardev. Its type is
>> specified via a field in the structure passed in argument.
>>
>> I didn't see other way than adding dedicated LSM hooks to achieve this,
>> but it is possible that their is a better way to do it?
>
> At the very least the hook should be made more general, and I'd have to
> see a proposal before commenting on that. security_dev_destroy(dev) might
> be a better approach. If there's reason to control destruction of vduse
> devices it's reasonable to assume that there are other devices with the
> same or similar properties.
VDUSE is different from other devices as the device is actually
implemented by the user-space application, so this is very specific in
my opinion.
>
> Since SELinux is your target use case, can you explain why you can't
> create SELinux policy to enforce the restrictions you're after? I believe
> (but can be proven wrong, of course) that SELinux has mechanism for dealing
> with controls on ioctls.
>
I am not aware of such mechanism to deal with ioctl(), if you have a
pointer that would be welcome.
Thanks,
Maxime
>
>>
>> Thanks,
>> Maxime
>>
>> [0]:
>> https://lore.kernel.org/all/[email protected]/
>>
>
On 10/24/2023 2:49 AM, Maxime Coquelin wrote:
>
>
> On 10/23/23 17:13, Casey Schaufler wrote:
>> On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
>>>
>>>
>>> On 10/21/23 00:20, Casey Schaufler wrote:
>>>> On 10/20/2023 8:58 AM, Maxime Coquelin 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.
>>>>
>>>> Why do you think that there needs to be a special LSM check for virtio
>>>> devices? What can't existing device attributes be used?
>>>
>>> Michael asked for a way for SELinux to allow/prevent the creation of
>>> some types of devices [0].
>>>
>>> A device is created using ioctl() on VDUSE control chardev. Its type is
>>> specified via a field in the structure passed in argument.
>>>
>>> I didn't see other way than adding dedicated LSM hooks to achieve this,
>>> but it is possible that their is a better way to do it?
>>
>> At the very least the hook should be made more general, and I'd have to
>> see a proposal before commenting on that. security_dev_destroy(dev)
>> might
>> be a better approach. If there's reason to control destruction of vduse
>> devices it's reasonable to assume that there are other devices with the
>> same or similar properties.
>
> VDUSE is different from other devices as the device is actually
> implemented by the user-space application, so this is very specific in
> my opinion.
This is hardly unique. If you're implementing the device
in user-space you may well be able to implement the desired
controls there.
>
>>
>> Since SELinux is your target use case, can you explain why you can't
>> create SELinux policy to enforce the restrictions you're after? I
>> believe
>> (but can be proven wrong, of course) that SELinux has mechanism for
>> dealing
>> with controls on ioctls.
>>
>
> I am not aware of such mechanism to deal with ioctl(), if you have a
> pointer that would be welcome.
security/selinux/hooks.c
>
> Thanks,
> Maxime
>
>>
>>>
>>> Thanks,
>>> Maxime
>>>
>>> [0]:
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>>
>>
>
On 10/24/23 17:30, Casey Schaufler wrote:
> On 10/24/2023 2:49 AM, Maxime Coquelin wrote:
>>
>>
>> On 10/23/23 17:13, Casey Schaufler wrote:
>>> On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 10/21/23 00:20, Casey Schaufler wrote:
>>>>> On 10/20/2023 8:58 AM, Maxime Coquelin 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.
>>>>>
>>>>> Why do you think that there needs to be a special LSM check for virtio
>>>>> devices? What can't existing device attributes be used?
>>>>
>>>> Michael asked for a way for SELinux to allow/prevent the creation of
>>>> some types of devices [0].
>>>>
>>>> A device is created using ioctl() on VDUSE control chardev. Its type is
>>>> specified via a field in the structure passed in argument.
>>>>
>>>> I didn't see other way than adding dedicated LSM hooks to achieve this,
>>>> but it is possible that their is a better way to do it?
>>>
>>> At the very least the hook should be made more general, and I'd have to
>>> see a proposal before commenting on that. security_dev_destroy(dev)
>>> might
>>> be a better approach. If there's reason to control destruction of vduse
>>> devices it's reasonable to assume that there are other devices with the
>>> same or similar properties.
>>
>> VDUSE is different from other devices as the device is actually
>> implemented by the user-space application, so this is very specific in
>> my opinion.
>
> This is hardly unique. If you're implementing the device
> in user-space you may well be able to implement the desired
> controls there.
>
>>
>>>
>>> Since SELinux is your target use case, can you explain why you can't
>>> create SELinux policy to enforce the restrictions you're after? I
>>> believe
>>> (but can be proven wrong, of course) that SELinux has mechanism for
>>> dealing
>>> with controls on ioctls.
>>>
>>
>> I am not aware of such mechanism to deal with ioctl(), if you have a
>> pointer that would be welcome.
>
> security/selinux/hooks.c
We might be able to extend selinux_file_ioctl(), but that will only
covers the ioctl for the control file, this patch also adds hook for the
device file opening that would need dedicated hook as the device type
information is stored in the device's private data.
Michael, before going further, I would be interested in your feedback.
Was this patch what you had in mind when requesting for a way to
allow/deny devices types for a given application?
Regards,
Maxime
>
>>
>> Thanks,
>> Maxime
>>
>>>
>>>>
>>>> Thanks,
>>>> Maxime
>>>>
>>>> [0]:
>>>> https://lore.kernel.org/all/[email protected]/
>>>>
>>>>
>>>
>>
>
On Thu, Nov 02, 2023 at 06:56:59PM +0100, Maxime Coquelin wrote:
>
>
> On 10/24/23 17:30, Casey Schaufler wrote:
> > On 10/24/2023 2:49 AM, Maxime Coquelin wrote:
> > >
> > >
> > > On 10/23/23 17:13, Casey Schaufler wrote:
> > > > On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
> > > > >
> > > > >
> > > > > On 10/21/23 00:20, Casey Schaufler wrote:
> > > > > > On 10/20/2023 8:58 AM, Maxime Coquelin 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.
> > > > > >
> > > > > > Why do you think that there needs to be a special LSM check for virtio
> > > > > > devices? What can't existing device attributes be used?
> > > > >
> > > > > Michael asked for a way for SELinux to allow/prevent the creation of
> > > > > some types of devices [0].
> > > > >
> > > > > A device is created using ioctl() on VDUSE control chardev. Its type is
> > > > > specified via a field in the structure passed in argument.
> > > > >
> > > > > I didn't see other way than adding dedicated LSM hooks to achieve this,
> > > > > but it is possible that their is a better way to do it?
> > > >
> > > > At the very least the hook should be made more general, and I'd have to
> > > > see a proposal before commenting on that. security_dev_destroy(dev)
> > > > might
> > > > be a better approach. If there's reason to control destruction of vduse
> > > > devices it's reasonable to assume that there are other devices with the
> > > > same or similar properties.
> > >
> > > VDUSE is different from other devices as the device is actually
> > > implemented by the user-space application, so this is very specific in
> > > my opinion.
> >
> > This is hardly unique. If you're implementing the device
> > in user-space you may well be able to implement the desired
> > controls there.
> >
> > >
> > > >
> > > > Since SELinux is your target use case, can you explain why you can't
> > > > create SELinux policy to enforce the restrictions you're after? I
> > > > believe
> > > > (but can be proven wrong, of course) that SELinux has mechanism for
> > > > dealing
> > > > with controls on ioctls.
> > > >
> > >
> > > I am not aware of such mechanism to deal with ioctl(), if you have a
> > > pointer that would be welcome.
> >
> > security/selinux/hooks.c
>
> We might be able to extend selinux_file_ioctl(), but that will only
> covers the ioctl for the control file, this patch also adds hook for the
> device file opening that would need dedicated hook as the device type
> information is stored in the device's private data.
>
> Michael, before going further, I would be interested in your feedback.
> Was this patch what you had in mind when requesting for a way to
> allow/deny devices types for a given application?
>
> Regards,
> Maxime
Yes, this is more or less what I had in mind.
> >
> > >
> > > Thanks,
> > > Maxime
> > >
> > > >
> > > > >
> > > > > Thanks,
> > > > > Maxime
> > > > >
> > > > > [0]:
> > > > > https://lore.kernel.org/all/[email protected]/
> > > > >
> > > > >
> > > >
> > >
> >
On 11/2/23 19:59, Michael S. Tsirkin wrote:
> On Thu, Nov 02, 2023 at 06:56:59PM +0100, Maxime Coquelin wrote:
>>
>>
>> On 10/24/23 17:30, Casey Schaufler wrote:
>>> On 10/24/2023 2:49 AM, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 10/23/23 17:13, Casey Schaufler wrote:
>>>>> On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
>>>>>>
>>>>>>
>>>>>> On 10/21/23 00:20, Casey Schaufler wrote:
>>>>>>> On 10/20/2023 8:58 AM, Maxime Coquelin 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.
>>>>>>>
>>>>>>> Why do you think that there needs to be a special LSM check for virtio
>>>>>>> devices? What can't existing device attributes be used?
>>>>>>
>>>>>> Michael asked for a way for SELinux to allow/prevent the creation of
>>>>>> some types of devices [0].
>>>>>>
>>>>>> A device is created using ioctl() on VDUSE control chardev. Its type is
>>>>>> specified via a field in the structure passed in argument.
>>>>>>
>>>>>> I didn't see other way than adding dedicated LSM hooks to achieve this,
>>>>>> but it is possible that their is a better way to do it?
>>>>>
>>>>> At the very least the hook should be made more general, and I'd have to
>>>>> see a proposal before commenting on that. security_dev_destroy(dev)
>>>>> might
>>>>> be a better approach. If there's reason to control destruction of vduse
>>>>> devices it's reasonable to assume that there are other devices with the
>>>>> same or similar properties.
>>>>
>>>> VDUSE is different from other devices as the device is actually
>>>> implemented by the user-space application, so this is very specific in
>>>> my opinion.
>>>
>>> This is hardly unique. If you're implementing the device
>>> in user-space you may well be able to implement the desired
>>> controls there.
>>>
>>>>
>>>>>
>>>>> Since SELinux is your target use case, can you explain why you can't
>>>>> create SELinux policy to enforce the restrictions you're after? I
>>>>> believe
>>>>> (but can be proven wrong, of course) that SELinux has mechanism for
>>>>> dealing
>>>>> with controls on ioctls.
>>>>>
>>>>
>>>> I am not aware of such mechanism to deal with ioctl(), if you have a
>>>> pointer that would be welcome.
>>>
>>> security/selinux/hooks.c
>>
>> We might be able to extend selinux_file_ioctl(), but that will only
>> covers the ioctl for the control file, this patch also adds hook for the
>> device file opening that would need dedicated hook as the device type
>> information is stored in the device's private data.
>>
>> Michael, before going further, I would be interested in your feedback.
>> Was this patch what you had in mind when requesting for a way to
>> allow/deny devices types for a given application?
>>
>> Regards,
>> Maxime
>
>
> Yes, this is more or less what I had in mind.
Great.
Do you think we need to cover both ioctl() on the control file and
open() on the device file, or only ioctl() is enough?
If the former, we will need VDUSE-specific hooks. I may be able to
improve my patch to have a single hook instead of 3 by passing the type
of operation as an extra argument (create/destroy/open).
If the latter, we may be able to extend the generic ioctl hook.
Personally, I think it would make sense to also ensure a given
application can only open existing VDUSE devices it supports. For
example, openvswitch should only be allowed to open networking VDUSE
devices.
Thanks,
Maxime
>
>>>
>>>>
>>>> Thanks,
>>>> Maxime
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Maxime
>>>>>>
>>>>>> [0]:
>>>>>> https://lore.kernel.org/all/[email protected]/
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
On Fri, Nov 03, 2023 at 08:55:19AM +0100, Maxime Coquelin wrote:
>
>
> On 11/2/23 19:59, Michael S. Tsirkin wrote:
> > On Thu, Nov 02, 2023 at 06:56:59PM +0100, Maxime Coquelin wrote:
> > >
> > >
> > > On 10/24/23 17:30, Casey Schaufler wrote:
> > > > On 10/24/2023 2:49 AM, Maxime Coquelin wrote:
> > > > >
> > > > >
> > > > > On 10/23/23 17:13, Casey Schaufler wrote:
> > > > > > On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 10/21/23 00:20, Casey Schaufler wrote:
> > > > > > > > On 10/20/2023 8:58 AM, Maxime Coquelin 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.
> > > > > > > >
> > > > > > > > Why do you think that there needs to be a special LSM check for virtio
> > > > > > > > devices? What can't existing device attributes be used?
> > > > > > >
> > > > > > > Michael asked for a way for SELinux to allow/prevent the creation of
> > > > > > > some types of devices [0].
> > > > > > >
> > > > > > > A device is created using ioctl() on VDUSE control chardev. Its type is
> > > > > > > specified via a field in the structure passed in argument.
> > > > > > >
> > > > > > > I didn't see other way than adding dedicated LSM hooks to achieve this,
> > > > > > > but it is possible that their is a better way to do it?
> > > > > >
> > > > > > At the very least the hook should be made more general, and I'd have to
> > > > > > see a proposal before commenting on that. security_dev_destroy(dev)
> > > > > > might
> > > > > > be a better approach. If there's reason to control destruction of vduse
> > > > > > devices it's reasonable to assume that there are other devices with the
> > > > > > same or similar properties.
> > > > >
> > > > > VDUSE is different from other devices as the device is actually
> > > > > implemented by the user-space application, so this is very specific in
> > > > > my opinion.
> > > >
> > > > This is hardly unique. If you're implementing the device
> > > > in user-space you may well be able to implement the desired
> > > > controls there.
> > > >
> > > > >
> > > > > >
> > > > > > Since SELinux is your target use case, can you explain why you can't
> > > > > > create SELinux policy to enforce the restrictions you're after? I
> > > > > > believe
> > > > > > (but can be proven wrong, of course) that SELinux has mechanism for
> > > > > > dealing
> > > > > > with controls on ioctls.
> > > > > >
> > > > >
> > > > > I am not aware of such mechanism to deal with ioctl(), if you have a
> > > > > pointer that would be welcome.
> > > >
> > > > security/selinux/hooks.c
> > >
> > > We might be able to extend selinux_file_ioctl(), but that will only
> > > covers the ioctl for the control file, this patch also adds hook for the
> > > device file opening that would need dedicated hook as the device type
> > > information is stored in the device's private data.
> > >
> > > Michael, before going further, I would be interested in your feedback.
> > > Was this patch what you had in mind when requesting for a way to
> > > allow/deny devices types for a given application?
> > >
> > > Regards,
> > > Maxime
> >
> >
> > Yes, this is more or less what I had in mind.
>
> Great.
>
> Do you think we need to cover both ioctl() on the control file and
> open() on the device file, or only ioctl() is enough?
>
> If the former, we will need VDUSE-specific hooks. I may be able to
> improve my patch to have a single hook instead of 3 by passing the type
> of operation as an extra argument (create/destroy/open).
>
> If the latter, we may be able to extend the generic ioctl hook.
>
> Personally, I think it would make sense to also ensure a given
> application can only open existing VDUSE devices it supports. For
> example, openvswitch should only be allowed to open networking VDUSE
> devices.
>
> Thanks,
> Maxime
I agree here. I think an open hook is important.
Make sure to document the need in the cover letter
and commit log.
> >
> > > >
> > > > >
> > > > > Thanks,
> > > > > Maxime
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Maxime
> > > > > > >
> > > > > > > [0]:
> > > > > > > https://lore.kernel.org/all/[email protected]/
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> >
Hello Paul,
On 11/8/23 03:31, Paul Moore wrote:
> On Oct 20, 2023 "Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
>> ---
>> 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 ...
No worries, I have also been busy these days.
>> 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.
I don't think we need a unique label for VDUSE devices, but maybe
Michael thinks otherwise?
>
>> +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.
Ack, I can remove the "dev" prefix in next revision.
>
> 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?
It is actually used, but maybe not in a correct way.
If you look at each hook, there are two checks performed:
1. Check for the operation type: create/destroy/open
2. Check for the device type: block/net
It means that the application will have to combine one (or more)
operation type with one (or more) device type.
Does that make sense?
Thanks,
Maxime
>
>> { NULL }
>> };
>>
>> --
>> 2.41.0
>
> --
> paul-moore.com
>
On Fri, Dec 08, 2023 at 12:01:15PM +0100, Maxime Coquelin wrote:
> Hello Paul,
>
> On 11/8/23 03:31, Paul Moore wrote:
> > On Oct 20, 2023 "Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 ...
>
> No worries, I have also been busy these days.
>
> > > 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.
>
> I don't think we need a unique label for VDUSE devices, but maybe
> Michael thinks otherwise?
I don't know.
All this is consumed by libvirt, you need to ask these guys.
> >
> > > +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.
>
> Ack, I can remove the "dev" prefix in next revision.
>
> >
> > 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?
>
> It is actually used, but maybe not in a correct way.
> If you look at each hook, there are two checks performed:
> 1. Check for the operation type: create/destroy/open
> 2. Check for the device type: block/net
>
> It means that the application will have to combine one (or more)
> operation type with one (or more) device type.
>
> Does that make sense?
>
> Thanks,
> Maxime
>
> >
> > > { NULL }
> > > };
> > > --
> > > 2.41.0
> >
> > --
> > paul-moore.com
> >
On 12/8/23 12:05, Michael S. Tsirkin wrote:
> On Fri, Dec 08, 2023 at 12:01:15PM +0100, Maxime Coquelin wrote:
>> Hello Paul,
>>
>> On 11/8/23 03:31, Paul Moore wrote:
>>> On Oct 20, 2023 "Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 ...
>>
>> No worries, I have also been busy these days.
>>
>>>> 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.
>>
>> I don't think we need a unique label for VDUSE devices, but maybe
>> Michael thinks otherwise?
>
> I don't know.
> All this is consumed by libvirt, you need to ask these guys.
I think it is not consumed by libvirt, at least not in the usecases I
have in mind. For networking devices, it will be consumed by OVS.
Maxime
On Fri, Dec 08, 2023 at 01:23:00PM +0100, Maxime Coquelin wrote:
>
>
> On 12/8/23 12:05, Michael S. Tsirkin wrote:
> > On Fri, Dec 08, 2023 at 12:01:15PM +0100, Maxime Coquelin wrote:
> > > Hello Paul,
> > >
> > > On 11/8/23 03:31, Paul Moore wrote:
> > > > On Oct 20, 2023 "Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 ...
> > >
> > > No worries, I have also been busy these days.
> > >
> > > > > 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.
> > >
> > > I don't think we need a unique label for VDUSE devices, but maybe
> > > Michael thinks otherwise?
> >
> > I don't know.
> > All this is consumed by libvirt, you need to ask these guys.
>
> I think it is not consumed by libvirt, at least not in the usecases I
> have in mind. For networking devices, it will be consumed by OVS.
>
> Maxime
OK, ovs then :)
--
MST
On 12/8/23 13:26, Michael S. Tsirkin wrote:
> On Fri, Dec 08, 2023 at 01:23:00PM +0100, Maxime Coquelin wrote:
>>
>>
>> On 12/8/23 12:05, Michael S. Tsirkin wrote:
>>> On Fri, Dec 08, 2023 at 12:01:15PM +0100, Maxime Coquelin wrote:
>>>> Hello Paul,
>>>>
>>>> On 11/8/23 03:31, Paul Moore wrote:
>>>>> On Oct 20, 2023 "Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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 ...
>>>>
>>>> No worries, I have also been busy these days.
>>>>
>>>>>> 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.
>>>>
>>>> I don't think we need a unique label for VDUSE devices, but maybe
>>>> Michael thinks otherwise?
>>>
>>> I don't know.
>>> All this is consumed by libvirt, you need to ask these guys.
>>
>> I think it is not consumed by libvirt, at least not in the usecases I
>> have in mind. For networking devices, it will be consumed by OVS.
>>
>> Maxime
>
> OK, ovs then :)
>
Adding Aaron, our go-to person for SELinux-related topics for OVS, but I
think we don't need to do relabeling for VDUSE chardevs.
Aaron, do you confirm?
Maxime