bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
@ 2020-04-03 17:55 Odin Ugedal
  2020-04-03 18:05 ` Odin Ugedal
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Odin Ugedal @ 2020-04-03 17:55 UTC (permalink / raw)
  To: bpf, linux-security-module, linux-kernel, tj,
	Harish.Kasiviswanathan, guro, amd-gfx
  Cc: 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 <odin@ugedal.com>
---
 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 <linux/fs.h>
-#include <linux/bpf-cgroup.h>
 
 #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 <linux/rcupdate.h>
 #include <linux/mutex.h>
 
+#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 devcgroup_check_permission(short type, u32 major, u32 minor, short access)
 	if (rc)
 		return -EPERM;
 
-	return __devcgroup_check_permission(type, major, minor, access);
+	#ifdef CONFIG_CGROUP_DEVICE
+	return devcgroup_legacy_check_permission(type, major, minor, access);
+
+	#else /* CONFIG_CGROUP_DEVICE */
+	return 0;
+
+	#endif /* CONFIG_CGROUP_DEVICE */
 }
 EXPORT_SYMBOL(devcgroup_check_permission);
+#endif /* defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF) */
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
  2020-04-03 17:55 [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code Odin Ugedal
@ 2020-04-03 18:05 ` Odin Ugedal
  2020-04-03 22:37 ` Roman Gushchin
  2020-04-13 18:43 ` Tejun Heo
  2 siblings, 0 replies; 5+ messages in thread
From: Odin Ugedal @ 2020-04-03 18:05 UTC (permalink / raw)
  To: bpf, linux-security-module, open list, Tejun Heo,
	Harish.Kasiviswanathan, guro, amd-gfx

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 <odin@ugedal.com>:
>
> 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 <odin@ugedal.com>
> ---
>  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 <linux/fs.h>
> -#include <linux/bpf-cgroup.h>
>
>  #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 <linux/rcupdate.h>
>  #include <linux/mutex.h>
>
> +#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 devcgroup_check_permission(short type, u32 major, u32 minor, short access)
>         if (rc)
>                 return -EPERM;
>
> -       return __devcgroup_check_permission(type, major, minor, access);
> +       #ifdef CONFIG_CGROUP_DEVICE
> +       return devcgroup_legacy_check_permission(type, major, minor, access);
> +
> +       #else /* CONFIG_CGROUP_DEVICE */
> +       return 0;
> +
> +       #endif /* CONFIG_CGROUP_DEVICE */
>  }
>  EXPORT_SYMBOL(devcgroup_check_permission);
> +#endif /* defined(CONFIG_CGROUP_DEVICE) || defined(CONFIG_CGROUP_BPF) */
> --
> 2.26.0
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
  2020-04-03 17:55 [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code Odin Ugedal
  2020-04-03 18:05 ` Odin Ugedal
@ 2020-04-03 22:37 ` Roman Gushchin
  2020-04-06 20:30   ` Daniel Borkmann
  2020-04-13 18:43 ` Tejun Heo
  2 siblings, 1 reply; 5+ messages in thread
From: Roman Gushchin @ 2020-04-03 22:37 UTC (permalink / raw)
  To: Odin Ugedal
  Cc: bpf, linux-security-module, linux-kernel, tj,
	Harish.Kasiviswanathan, amd-gfx

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 <odin@ugedal.com>

Hello, Odin!

The patch makes perfect sense to me.

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
  2020-04-03 22:37 ` Roman Gushchin
@ 2020-04-06 20:30   ` Daniel Borkmann
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Borkmann @ 2020-04-06 20:30 UTC (permalink / raw)
  To: Roman Gushchin, Odin Ugedal
  Cc: bpf, linux-security-module, linux-kernel, tj,
	Harish.Kasiviswanathan, amd-gfx

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 <odin@ugedal.com>
> 
> The patch makes perfect sense to me.
> 
> Acked-by: Roman Gushchin <guro@fb.com>

Tejun, I presume you'll pick this up (given the files this fix touches)?

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code
  2020-04-03 17:55 [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code Odin Ugedal
  2020-04-03 18:05 ` Odin Ugedal
  2020-04-03 22:37 ` Roman Gushchin
@ 2020-04-13 18:43 ` Tejun Heo
  2 siblings, 0 replies; 5+ messages in thread
From: Tejun Heo @ 2020-04-13 18:43 UTC (permalink / raw)
  To: Odin Ugedal
  Cc: bpf, linux-security-module, linux-kernel, Harish.Kasiviswanathan,
	guro, amd-gfx

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-04-13 18:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-03 17:55 [PATCH] device_cgroup: Cleanup cgroup eBPF device filter code Odin Ugedal
2020-04-03 18:05 ` Odin Ugedal
2020-04-03 22:37 ` Roman Gushchin
2020-04-06 20:30   ` Daniel Borkmann
2020-04-13 18:43 ` Tejun Heo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).