Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
On Fri, Apr 03, 2020 at 07:55:28PM +0200, Odin Ugedal wrote:
> Original cgroup v2 eBPF code for filtering device access made it
> possible to compile with CONFIG_CGROUP_DEVICE=n and still use the eBPF
> filtering. Change
> commit 4b7d4d453fc4 ("device_cgroup: Export devcgroup_check_permission")
> reverted this, making it required to set it to y.
>
> Since the device filtering (and all the docs) for cgroup v2 is no longer
> a "device controller" like it was in v1, someone might compile their
> kernel with CONFIG_CGROUP_DEVICE=n. Then (for linux 5.5+) the eBPF
> filter will not be invoked, and all processes will be allowed access
> to all devices, no matter what the eBPF filter says.
Applied to cgroup/for-5.7-fixes.
Thanks.
--
tejun
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
On 4/4/20 12:37 AM, Roman Gushchin wrote:
On Fri, Apr 03, 2020 at 07:55:28PM +0200, Odin Ugedal wrote:
Original cgroup v2 eBPF code for filtering device access made it
possible to compile with CONFIG_CGROUP_DEVICE=n and still use the eBPF
filtering. Change
commit 4b7d4d453fc4 ("device_cgroup: Export devcgroup_check_permission")
reverted this, making it required to set it to y.
Since the device filtering (and all the docs) for cgroup v2 is no longer
a "device controller" like it was in v1, someone might compile their
kernel with CONFIG_CGROUP_DEVICE=n. Then (for linux 5.5+) the eBPF
filter will not be invoked, and all processes will be allowed access
to all devices, no matter what the eBPF filter says.
Signed-off-by: Odin Ugedal
The patch makes perfect sense to me.
Acked-by: Roman Gushchin
Tejun, I presume you'll pick this up (given the files this fix touches)?
Thanks,
Daniel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
Hi (patch author here),
This is my first "real" patch, so looking forward to some feedback! I am
not sure if this behavior is the "best one", or if we should require
CONFIG_CGROUP_DEVICE to be set to yes. In that case we can just abandon
this patch and replace the original "#if defined(CONFIG_CGROUP_DEVICE) ||
defined(CONFIG_CGROUP_BPF)" with a simple "#ifdef CONFIG_CGROUP_DEVICE" and
update the docs and the config.
It is also another alternative to keep the code in this patch and move some
of it into a separate file in order to avoid all the ifdefs, and to make
the split between cgroup v1 and cgroup v2 code cleaner.
As a reference, only Fedora is currently shipping with cgroup v2 as
default (afaik.) and their kernel is compiled (5.3.7-301.fc31.x86_64)
with CONFIG_CGROUP_DEVICE=y and CONFIG_CGROUP_BPF=y, so this will not
affect them.
Odin Ugedal
fre. 3. apr. 2020 kl. 19:55 skrev Odin Ugedal :
> Original cgroup v2 eBPF code for filtering device access made it
> possible to compile with CONFIG_CGROUP_DEVICE=n and still use the eBPF
> filtering. Change
> commit 4b7d4d453fc4 ("device_cgroup: Export devcgroup_check_permission")
> reverted this, making it required to set it to y.
>
> Since the device filtering (and all the docs) for cgroup v2 is no longer
> a "device controller" like it was in v1, someone might compile their
> kernel with CONFIG_CGROUP_DEVICE=n. Then (for linux 5.5+) the eBPF
> filter will not be invoked, and all processes will be allowed access
> to all devices, no matter what the eBPF filter says.
>
> Signed-off-by: Odin Ugedal
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +-
> include/linux/device_cgroup.h | 14 +-
> security/Makefile | 2 +-
> security/device_cgroup.c | 19 ---
> 4 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 4a3049841086..c24cad3c64ed 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -1050,7 +1050,7 @@ void kfd_dec_compute_active(struct kfd_dev *dev);
> /* Check with device cgroup if @kfd device is accessible */
> static inline int kfd_devcgroup_check_permission(struct kfd_dev *kfd)
> {
> -#if defined(CONFIG_CGROUP_DEVICE)
> +#if defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)
> struct drm_device *ddev = kfd->ddev;
>
> return devcgroup_check_permission(DEVCG_DEV_CHAR,
> ddev->driver->major,
> diff --git a/include/linux/device_cgroup.h b/include/linux/device_cgroup.h
> index fa35b52e0002..9a72214496e5 100644
> --- a/include/linux/device_cgroup.h
> +++ b/include/linux/device_cgroup.h
> @@ -1,6 +1,5 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #include
> -#include
>
> #define DEVCG_ACC_MKNOD 1
> #define DEVCG_ACC_READ 2
> @@ -11,16 +10,10 @@
> #define DEVCG_DEV_CHAR 2
> #define DEVCG_DEV_ALL 4 /* this represents all devices */
>
> -#ifdef CONFIG_CGROUP_DEVICE
> -int devcgroup_check_permission(short type, u32 major, u32 minor,
> - short access);
> -#else
> -static inline int devcgroup_check_permission(short type, u32 major, u32
> minor,
> -short access)
> -{ return 0; }
> -#endif
>
> #if defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)
> +int devcgroup_check_permission(short type, u32 major, u32 minor,
> + short access);
> static inline int devcgroup_inode_permission(struct inode *inode, int
> mask)
> {
> short type, access = 0;
> @@ -61,6 +54,9 @@ static inline int devcgroup_inode_mknod(int mode, dev_t
> dev)
> }
>
> #else
> +static inline int devcgroup_check_permission(short type, u32 major, u32
> minor,
> + short access)
> +{ return 0; }
> static inline int devcgroup_inode_permission(struct inode *inode, int
> mask)
> { return 0; }
> static inline int devcgroup_inode_mknod(int mode, dev_t dev)
> diff --git a/security/Makefile b/security/Makefile
> index 22e73a3482bd..3baf435de541 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -30,7 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
> obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)+= lockdown/
> -obj-$(CONFIG_CGROUP_DEVICE)+= device_cgroup.o
> +obj-$(CONFIG_CGROUPS) += device_cgroup.o
> obj-$(CONFIG_BPF_LSM) += bpf/
>
> # Object integrity file lists
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index 7d0f8f7431ff..43ab0ad45c1b 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -15,6 +15,8 @@
> #include
> #include
>
> +#ifdef CONFIG_CGROUP_DEVICE
> +
> static DEFINE_MUTEX(devcgroup_mutex);
>
> enum devcg_behavior {
> @@ -792,7 +794,7 @@ struct cgroup_subsys
Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
Hi (patch author here),
This is my first "real" patch, so looking forward to some feedback! I
am not sure if this behavior is the "best one", or if we should
require CONFIG_CGROUP_DEVICE to be set to yes. In that case we can
just abandon this patch and replace the original "#if
defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)" with a
simple "#ifdef CONFIG_CGROUP_DEVICE" and update the docs and the
config.
It is also another alternative to keep the code in this patch and move
some of it into a separate file in order to avoid all the ifdefs, and
to make the split between cgroup v1 and cgroup v2 code cleaner.
As a reference, only Fedora is currently shipping with cgroup v2 as
default (afaik.) and their kernel is compiled (5.3.7-301.fc31.x86_64)
with CONFIG_CGROUP_DEVICE=y and CONFIG_CGROUP_BPF=y, so this will not
affect them.
Odin Ugedal
fre. 3. apr. 2020 kl. 19:55 skrev Odin Ugedal :
>
> Original cgroup v2 eBPF code for filtering device access made it
> possible to compile with CONFIG_CGROUP_DEVICE=n and still use the eBPF
> filtering. Change
> commit 4b7d4d453fc4 ("device_cgroup: Export devcgroup_check_permission")
> reverted this, making it required to set it to y.
>
> Since the device filtering (and all the docs) for cgroup v2 is no longer
> a "device controller" like it was in v1, someone might compile their
> kernel with CONFIG_CGROUP_DEVICE=n. Then (for linux 5.5+) the eBPF
> filter will not be invoked, and all processes will be allowed access
> to all devices, no matter what the eBPF filter says.
>
> Signed-off-by: Odin Ugedal
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +-
> include/linux/device_cgroup.h | 14 +-
> security/Makefile | 2 +-
> security/device_cgroup.c | 19 ---
> 4 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 4a3049841086..c24cad3c64ed 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -1050,7 +1050,7 @@ void kfd_dec_compute_active(struct kfd_dev *dev);
> /* Check with device cgroup if @kfd device is accessible */
> static inline int kfd_devcgroup_check_permission(struct kfd_dev *kfd)
> {
> -#if defined(CONFIG_CGROUP_DEVICE)
> +#if defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)
> struct drm_device *ddev = kfd->ddev;
>
> return devcgroup_check_permission(DEVCG_DEV_CHAR, ddev->driver->major,
> diff --git a/include/linux/device_cgroup.h b/include/linux/device_cgroup.h
> index fa35b52e0002..9a72214496e5 100644
> --- a/include/linux/device_cgroup.h
> +++ b/include/linux/device_cgroup.h
> @@ -1,6 +1,5 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #include
> -#include
>
> #define DEVCG_ACC_MKNOD 1
> #define DEVCG_ACC_READ 2
> @@ -11,16 +10,10 @@
> #define DEVCG_DEV_CHAR 2
> #define DEVCG_DEV_ALL 4 /* this represents all devices */
>
> -#ifdef CONFIG_CGROUP_DEVICE
> -int devcgroup_check_permission(short type, u32 major, u32 minor,
> - short access);
> -#else
> -static inline int devcgroup_check_permission(short type, u32 major, u32
> minor,
> -short access)
> -{ return 0; }
> -#endif
>
> #if defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)
> +int devcgroup_check_permission(short type, u32 major, u32 minor,
> + short access);
> static inline int devcgroup_inode_permission(struct inode *inode, int mask)
> {
> short type, access = 0;
> @@ -61,6 +54,9 @@ static inline int devcgroup_inode_mknod(int mode, dev_t dev)
> }
>
> #else
> +static inline int devcgroup_check_permission(short type, u32 major, u32
> minor,
> + short access)
> +{ return 0; }
> static inline int devcgroup_inode_permission(struct inode *inode, int mask)
> { return 0; }
> static inline int devcgroup_inode_mknod(int mode, dev_t dev)
> diff --git a/security/Makefile b/security/Makefile
> index 22e73a3482bd..3baf435de541 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -30,7 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
> obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)+= lockdown/
> -obj-$(CONFIG_CGROUP_DEVICE)+= device_cgroup.o
> +obj-$(CONFIG_CGROUPS) += device_cgroup.o
> obj-$(CONFIG_BPF_LSM) += bpf/
>
> # Object integrity file lists
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index 7d0f8f7431ff..43ab0ad45c1b 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -15,6 +15,8 @@
> #include
> #include
>
> +#ifdef CONFIG_CGROUP_DEVICE
> +
> static DEFINE_MUTEX(devcgroup_mutex);
>
> enum devcg_behavior {
> @@ -792,7 +794,7 @@ struct cgroup_subsys
[PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
Original cgroup v2 eBPF code for filtering device access made it
possible to compile with CONFIG_CGROUP_DEVICE=n and still use the eBPF
filtering. Change
commit 4b7d4d453fc4 ("device_cgroup: Export devcgroup_check_permission")
reverted this, making it required to set it to y.
Since the device filtering (and all the docs) for cgroup v2 is no longer
a "device controller" like it was in v1, someone might compile their
kernel with CONFIG_CGROUP_DEVICE=n. Then (for linux 5.5+) the eBPF
filter will not be invoked, and all processes will be allowed access
to all devices, no matter what the eBPF filter says.
Signed-off-by: Odin Ugedal
---
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +-
include/linux/device_cgroup.h | 14 +-
security/Makefile | 2 +-
security/device_cgroup.c | 19 ---
4 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 4a3049841086..c24cad3c64ed 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -1050,7 +1050,7 @@ void kfd_dec_compute_active(struct kfd_dev *dev);
/* Check with device cgroup if @kfd device is accessible */
static inline int kfd_devcgroup_check_permission(struct kfd_dev *kfd)
{
-#if defined(CONFIG_CGROUP_DEVICE)
+#if defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)
struct drm_device *ddev = kfd->ddev;
return devcgroup_check_permission(DEVCG_DEV_CHAR, ddev->driver->major,
diff --git a/include/linux/device_cgroup.h b/include/linux/device_cgroup.h
index fa35b52e0002..9a72214496e5 100644
--- a/include/linux/device_cgroup.h
+++ b/include/linux/device_cgroup.h
@@ -1,6 +1,5 @@
/* SPDX-License-Identifier: GPL-2.0 */
#include
-#include
#define DEVCG_ACC_MKNOD 1
#define DEVCG_ACC_READ 2
@@ -11,16 +10,10 @@
#define DEVCG_DEV_CHAR 2
#define DEVCG_DEV_ALL 4 /* this represents all devices */
-#ifdef CONFIG_CGROUP_DEVICE
-int devcgroup_check_permission(short type, u32 major, u32 minor,
- short access);
-#else
-static inline int devcgroup_check_permission(short type, u32 major, u32 minor,
-short access)
-{ return 0; }
-#endif
#if defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)
+int devcgroup_check_permission(short type, u32 major, u32 minor,
+ short access);
static inline int devcgroup_inode_permission(struct inode *inode, int mask)
{
short type, access = 0;
@@ -61,6 +54,9 @@ static inline int devcgroup_inode_mknod(int mode, dev_t dev)
}
#else
+static inline int devcgroup_check_permission(short type, u32 major, u32 minor,
+ short access)
+{ return 0; }
static inline int devcgroup_inode_permission(struct inode *inode, int mask)
{ return 0; }
static inline int devcgroup_inode_mknod(int mode, dev_t dev)
diff --git a/security/Makefile b/security/Makefile
index 22e73a3482bd..3baf435de541 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -30,7 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)+= lockdown/
-obj-$(CONFIG_CGROUP_DEVICE)+= device_cgroup.o
+obj-$(CONFIG_CGROUPS) += device_cgroup.o
obj-$(CONFIG_BPF_LSM) += bpf/
# Object integrity file lists
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 7d0f8f7431ff..43ab0ad45c1b 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -15,6 +15,8 @@
#include
#include
+#ifdef CONFIG_CGROUP_DEVICE
+
static DEFINE_MUTEX(devcgroup_mutex);
enum devcg_behavior {
@@ -792,7 +794,7 @@ struct cgroup_subsys devices_cgrp_subsys = {
};
/**
- * __devcgroup_check_permission - checks if an inode operation is permitted
+ * devcgroup_legacy_check_permission - checks if an inode operation is
permitted
* @dev_cgroup: the dev cgroup to be tested against
* @type: device type
* @major: device major number
@@ -801,7 +803,7 @@ struct cgroup_subsys devices_cgrp_subsys = {
*
* returns 0 on success, -EPERM case the operation is not permitted
*/
-static int __devcgroup_check_permission(short type, u32 major, u32 minor,
+static int devcgroup_legacy_check_permission(short type, u32 major, u32 minor,
short access)
{
struct dev_cgroup *dev_cgroup;
@@ -825,6 +827,10 @@ static int __devcgroup_check_permission(short type, u32
major, u32 minor,
return 0;
}
+#endif /* CONFIG_CGROUP_DEVICE */
+
+#if defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF)
+
int devcgroup_check_permission(short type, u32 major, u32 minor, short access)
{
int rc = BPF_CGROUP_RUN_PROG_DEVICE_CGROUP(type, major, minor, access);
@@ -832,6 +838,13 @@ int
