[PATCH 0/4] Track exported dma-buffers with memcg

[PATCH 0/4] Track exported dma-buffers with memcg

[T.J. Mercier]

Based on discussions at LPC, this series adds a memory.stat counter for
exported dmabufs. This counter allows us to continue tracking
system-wide total exported buffer sizes which there is no longer any
way to get without DMABUF_SYSFS_STATS, and adds a new capability to
track per-cgroup exported buffer sizes. The total (root counter) is
helpful for accounting in-kernel dmabuf use (by comparing with the sum
of child nodes or with the sum of sizes of mapped buffers or FD
references in procfs) in addition to helping identify driver memory
leaks when in-kernel use continually increases over time. With
per-application cgroups, the per-cgroup counter allows us to quickly
see how much dma-buf memory an application has caused to be allocated.
This avoids the need to read through all of procfs which can be a
lengthy process, and causes the charge to "stick" to the allocating
process/cgroup as long as the buffer is alive, regardless of how the
buffer is shared (unless the charge is transferred).

The first patch adds the counter to memcg. The next two patches allow
the charge for a buffer to be transferred across cgroups which is
necessary because of the way most dmabufs are allocated from a central
process on Android. The fourth patch adds a SELinux hook to binder in
order to control who is allowed to transfer buffer charges.

[1] https://lore.kernel.org/all/20220617085702.4298-1-christian.koenig@amd.com/

Hridya Valsaraju (1):
  binder: Add flags to relinquish ownership of fds

T.J. Mercier (3):
  memcg: Track exported dma-buffers
  dmabuf: Add cgroup charge transfer function
  security: binder: Add transfer_charge SElinux hook

 Documentation/admin-guide/cgroup-v2.rst |  5 +++
 drivers/android/binder.c                | 36 +++++++++++++++--
 drivers/dma-buf/dma-buf.c               | 54 +++++++++++++++++++++++--
 include/linux/dma-buf.h                 |  5 +++
 include/linux/lsm_hook_defs.h           |  2 +
 include/linux/lsm_hooks.h               |  6 +++
 include/linux/memcontrol.h              |  7 ++++
 include/linux/security.h                |  2 +
 include/uapi/linux/android/binder.h     | 23 +++++++++--
 mm/memcontrol.c                         |  4 ++
 security/security.c                     |  6 +++
 security/selinux/hooks.c                |  9 +++++
 security/selinux/include/classmap.h     |  2 +-
 13 files changed, 149 insertions(+), 12 deletions(-)


base-commit: b7bfaa761d760e72a969d116517eaa12e404c262
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[T.J. Mercier]

Any process can cause a memory charge transfer to occur to any other
process when transmitting a file descriptor through binder. This should
only be possible for central allocator processes, so a new SELinux
permission is added to restrict which processes are allowed to initiate
these charge transfers.

Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 drivers/android/binder.c            | 5 +++++
 include/linux/lsm_hook_defs.h       | 2 ++
 include/linux/lsm_hooks.h           | 6 ++++++
 include/linux/security.h            | 2 ++
 security/security.c                 | 6 ++++++
 security/selinux/hooks.c            | 9 +++++++++
 security/selinux/include/classmap.h | 2 +-
 7 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9830848c8d25..9063db04826d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
 	if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
 		struct dma_buf *dmabuf;
 
+		if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
+			ret = -EPERM;
+			goto err_security;
+		}
+
 		if (unlikely(!is_dma_buf_file(file))) {
 			binder_user_error(
 				"%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ed6cb2ac55fa..8db2a958557e 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -33,6 +33,8 @@ LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
 	 const struct cred *to)
 LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
 	 const struct cred *to, struct file *file)
+LSM_HOOK(int, 0, binder_transfer_charge, const struct cred *from,
+	 const struct cred *to)
 LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
 	 unsigned int mode)
 LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 0a5ba81f7367..39c40c7bf519 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1385,6 +1385,12 @@
  *	@file contains the struct file being transferred.
  *	@to contains the struct cred for the receiving process.
  *	Return 0 if permission is granted.
+ * @binder_transfer_charge:
+ *	Check whether @from is allowed to transfer the memory charge for a
+ *	buffer out of its cgroup to @to.
+ *	@from contains the struct cred for the sending process.
+ *	@to contains the struct cred for the receiving process.
+ *	Return 0 if permission is granted.
  *
  * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
diff --git a/include/linux/security.h b/include/linux/security.h
index 5b67f208f7de..3b7472308430 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -270,6 +270,8 @@ int security_binder_transfer_binder(const struct cred *from,
 				    const struct cred *to);
 int security_binder_transfer_file(const struct cred *from,
 				  const struct cred *to, struct file *file);
+int security_binder_transfer_charge(const struct cred *from,
+				    const struct cred *to);
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
diff --git a/security/security.c b/security/security.c
index d1571900a8c7..97e1e74d1ff2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -801,6 +801,12 @@ int security_binder_transfer_file(const struct cred *from,
 	return call_int_hook(binder_transfer_file, 0, from, to, file);
 }
 
+int security_binder_transfer_charge(const struct cred *from,
+				    const struct cred *to)
+{
+	return call_int_hook(binder_transfer_charge, 0, from, to);
+}
+
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	return call_int_hook(ptrace_access_check, 0, child, mode);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3c5be76a9199..823ef14924bd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
 			    &ad);
 }
 
+static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
+{
+	return avc_has_perm(&selinux_state,
+			    cred_sid(from), cred_sid(to),
+			    SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
+			    NULL);
+}
+
 static int selinux_ptrace_access_check(struct task_struct *child,
 				       unsigned int mode)
 {
@@ -7052,6 +7060,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
 	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
 	LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
+	LSM_HOOK_INIT(binder_transfer_charge, selinux_binder_transfer_charge),
 
 	LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index a3c380775d41..2eef180d10d7 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
 	{ "tun_socket",
 	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },
 	{ "binder", { "impersonate", "call", "set_context_mgr", "transfer",
-		      NULL } },
+		      "transfer_charge", NULL } },
 	{ "cap_userns",
 	  { COMMON_CAP_PERMS, NULL } },
 	{ "cap2_userns",
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[Casey Schaufler]

On 1/9/2023 1:38 PM, T.J. Mercier wrote:
> Any process can cause a memory charge transfer to occur to any other
> process when transmitting a file descriptor through binder. This should
> only be possible for central allocator processes,

How is a "central allocator process" identified? If I have a LSM that
is not SELinux (e.g. AppArmor, Smack) or no LSM at all, how can/should this
be enforced? Why isn't binder enforcing this restriction itself?

>  so a new SELinux
> permission is added to restrict which processes are allowed to initiate
> these charge transfers.

Which is all perfectly reasonable if you have SELinux.

>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> ---
>  drivers/android/binder.c            | 5 +++++
>  include/linux/lsm_hook_defs.h       | 2 ++
>  include/linux/lsm_hooks.h           | 6 ++++++
>  include/linux/security.h            | 2 ++
>  security/security.c                 | 6 ++++++
>  security/selinux/hooks.c            | 9 +++++++++
>  security/selinux/include/classmap.h | 2 +-
>  7 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 9830848c8d25..9063db04826d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
>  	if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
>  		struct dma_buf *dmabuf;
>  
> +		if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> +			ret = -EPERM;
> +			goto err_security;
> +		}
> +
>  		if (unlikely(!is_dma_buf_file(file))) {
>  			binder_user_error(
>  				"%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ed6cb2ac55fa..8db2a958557e 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -33,6 +33,8 @@ LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
>  	 const struct cred *to)
>  LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
>  	 const struct cred *to, struct file *file)
> +LSM_HOOK(int, 0, binder_transfer_charge, const struct cred *from,
> +	 const struct cred *to)
>  LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
>  	 unsigned int mode)
>  LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 0a5ba81f7367..39c40c7bf519 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1385,6 +1385,12 @@
>   *	@file contains the struct file being transferred.
>   *	@to contains the struct cred for the receiving process.
>   *	Return 0 if permission is granted.
> + * @binder_transfer_charge:
> + *	Check whether @from is allowed to transfer the memory charge for a
> + *	buffer out of its cgroup to @to.
> + *	@from contains the struct cred for the sending process.
> + *	@to contains the struct cred for the receiving process.
> + *	Return 0 if permission is granted.
>   *
>   * @ptrace_access_check:
>   *	Check permission before allowing the current process to trace the
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5b67f208f7de..3b7472308430 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -270,6 +270,8 @@ int security_binder_transfer_binder(const struct cred *from,
>  				    const struct cred *to);
>  int security_binder_transfer_file(const struct cred *from,
>  				  const struct cred *to, struct file *file);
> +int security_binder_transfer_charge(const struct cred *from,
> +				    const struct cred *to);
>  int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
>  int security_ptrace_traceme(struct task_struct *parent);
>  int security_capget(struct task_struct *target,
> diff --git a/security/security.c b/security/security.c
> index d1571900a8c7..97e1e74d1ff2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -801,6 +801,12 @@ int security_binder_transfer_file(const struct cred *from,
>  	return call_int_hook(binder_transfer_file, 0, from, to, file);
>  }
>  
> +int security_binder_transfer_charge(const struct cred *from,
> +				    const struct cred *to)
> +{
> +	return call_int_hook(binder_transfer_charge, 0, from, to);
> +}
> +
>  int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
>  {
>  	return call_int_hook(ptrace_access_check, 0, child, mode);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3c5be76a9199..823ef14924bd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
>  			    &ad);
>  }
>  
> +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> +{
> +	return avc_has_perm(&selinux_state,
> +			    cred_sid(from), cred_sid(to),
> +			    SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> +			    NULL);
> +}
> +
>  static int selinux_ptrace_access_check(struct task_struct *child,
>  				       unsigned int mode)
>  {
> @@ -7052,6 +7060,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>  	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
>  	LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
> +	LSM_HOOK_INIT(binder_transfer_charge, selinux_binder_transfer_charge),
>  
>  	LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index a3c380775d41..2eef180d10d7 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
>  	{ "tun_socket",
>  	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },
>  	{ "binder", { "impersonate", "call", "set_context_mgr", "transfer",
> -		      NULL } },
> +		      "transfer_charge", NULL } },
>  	{ "cap_userns",
>  	  { COMMON_CAP_PERMS, NULL } },
>  	{ "cap2_userns",

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[T.J. Mercier]

On Mon, Jan 9, 2023 at 2:28 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 1/9/2023 1:38 PM, T.J. Mercier wrote:
> > Any process can cause a memory charge transfer to occur to any other
> > process when transmitting a file descriptor through binder. This should
> > only be possible for central allocator processes,
>
> How is a "central allocator process" identified?

Any process with the transfer_charge permission. On Android this is
the graphics allocator HAL which would have this added to its policy.

> If I have a LSM that
> is not SELinux (e.g. AppArmor, Smack) or no LSM at all, how can/should this
> be enforced?

Sorry, why would you be expecting enforcement with no LSM? Are you
suggesting that this check should be different than the ones that
already exist for Binder here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/lsm_hook_defs.h#n29

> Why isn't binder enforcing this restriction itself?

Binder has no direct knowledge of which process has been designated as
an allocator / charge transferrer. That is defined externally by
whoever configures the system.

> >  so a new SELinux
> > permission is added to restrict which processes are allowed to initiate
> > these charge transfers.
>
> Which is all perfectly reasonable if you have SELinux.
>
> >
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > ---
> >  drivers/android/binder.c            | 5 +++++
> >  include/linux/lsm_hook_defs.h       | 2 ++
> >  include/linux/lsm_hooks.h           | 6 ++++++
> >  include/linux/security.h            | 2 ++
> >  security/security.c                 | 6 ++++++
> >  security/selinux/hooks.c            | 9 +++++++++
> >  security/selinux/include/classmap.h | 2 +-
> >  7 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 9830848c8d25..9063db04826d 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> >       if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> >               struct dma_buf *dmabuf;
> >
> > +             if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> > +                     ret = -EPERM;
> > +                     goto err_security;
> > +             }
> > +
> >               if (unlikely(!is_dma_buf_file(file))) {
> >                       binder_user_error(
> >                               "%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index ed6cb2ac55fa..8db2a958557e 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -33,6 +33,8 @@ LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
> >        const struct cred *to)
> >  LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
> >        const struct cred *to, struct file *file)
> > +LSM_HOOK(int, 0, binder_transfer_charge, const struct cred *from,
> > +      const struct cred *to)
> >  LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
> >        unsigned int mode)
> >  LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 0a5ba81f7367..39c40c7bf519 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1385,6 +1385,12 @@
> >   *   @file contains the struct file being transferred.
> >   *   @to contains the struct cred for the receiving process.
> >   *   Return 0 if permission is granted.
> > + * @binder_transfer_charge:
> > + *   Check whether @from is allowed to transfer the memory charge for a
> > + *   buffer out of its cgroup to @to.
> > + *   @from contains the struct cred for the sending process.
> > + *   @to contains the struct cred for the receiving process.
> > + *   Return 0 if permission is granted.
> >   *
> >   * @ptrace_access_check:
> >   *   Check permission before allowing the current process to trace the
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 5b67f208f7de..3b7472308430 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -270,6 +270,8 @@ int security_binder_transfer_binder(const struct cred *from,
> >                                   const struct cred *to);
> >  int security_binder_transfer_file(const struct cred *from,
> >                                 const struct cred *to, struct file *file);
> > +int security_binder_transfer_charge(const struct cred *from,
> > +                                 const struct cred *to);
> >  int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
> >  int security_ptrace_traceme(struct task_struct *parent);
> >  int security_capget(struct task_struct *target,
> > diff --git a/security/security.c b/security/security.c
> > index d1571900a8c7..97e1e74d1ff2 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -801,6 +801,12 @@ int security_binder_transfer_file(const struct cred *from,
> >       return call_int_hook(binder_transfer_file, 0, from, to, file);
> >  }
> >
> > +int security_binder_transfer_charge(const struct cred *from,
> > +                                 const struct cred *to)
> > +{
> > +     return call_int_hook(binder_transfer_charge, 0, from, to);
> > +}
> > +
> >  int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
> >  {
> >       return call_int_hook(ptrace_access_check, 0, child, mode);
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3c5be76a9199..823ef14924bd 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
> >                           &ad);
> >  }
> >
> > +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> > +{
> > +     return avc_has_perm(&selinux_state,
> > +                         cred_sid(from), cred_sid(to),
> > +                         SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> > +                         NULL);
> > +}
> > +
> >  static int selinux_ptrace_access_check(struct task_struct *child,
> >                                      unsigned int mode)
> >  {
> > @@ -7052,6 +7060,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> >       LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> >       LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
> >       LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
> > +     LSM_HOOK_INIT(binder_transfer_charge, selinux_binder_transfer_charge),
> >
> >       LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
> >       LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
> > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> > index a3c380775d41..2eef180d10d7 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
> >       { "tun_socket",
> >         { COMMON_SOCK_PERMS, "attach_queue", NULL } },
> >       { "binder", { "impersonate", "call", "set_context_mgr", "transfer",
> > -                   NULL } },
> > +                   "transfer_charge", NULL } },
> >       { "cap_userns",
> >         { COMMON_CAP_PERMS, NULL } },
> >       { "cap2_userns",

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[Casey Schaufler]

On 1/9/2023 4:30 PM, T.J. Mercier wrote:
> On Mon, Jan 9, 2023 at 2:28 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/9/2023 1:38 PM, T.J. Mercier wrote:
>>> Any process can cause a memory charge transfer to occur to any other
>>> process when transmitting a file descriptor through binder. This should
>>> only be possible for central allocator processes,
>> How is a "central allocator process" identified?
> Any process with the transfer_charge permission. On Android this is
> the graphics allocator HAL which would have this added to its policy.

OK. You're putting SELinux policy directly into the name of the LSM hook.

>
>> If I have a LSM that
>> is not SELinux (e.g. AppArmor, Smack) or no LSM at all, how can/should this
>> be enforced?
> Sorry, why would you be expecting enforcement with no LSM?

Because the LSM is supposed to be a set of *additional* restrictions.
If there are no restrictions when there's no LSM, you can't add to
existing restrictions. If binder works correctly without any restrictions
that's fine. It seems odd that you'd add SELinux restrictions if there
are no basic restrictions. If, on the other hand, binder doesn't have
native restrictions because it always assumes SELinux, it ought to have
a CONFIG dependency on SELinux.

None of which is really important.

>  Are you
> suggesting that this check should be different than the ones that
> already exist for Binder here?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/lsm_hook_defs.h#n29

This one seems just a little bit more like an implementation of
SELinux policy in the hook than some of the others. If there is no way
to identify the special process without SELinux policy it's going to
be really difficult for a different LSM to utilize the hook.

>
>> Why isn't binder enforcing this restriction itself?
> Binder has no direct knowledge of which process has been designated as
> an allocator / charge transferrer. That is defined externally by
> whoever configures the system.

So the attribute isn't a binder attribute, it's an SELinux attribute?
That isn't appropriate in the LSM interface, at least not explicitly.

>
>>>  so a new SELinux
>>> permission is added to restrict which processes are allowed to initiate
>>> these charge transfers.
>> Which is all perfectly reasonable if you have SELinux.
>>
>>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
>>> ---
>>>  drivers/android/binder.c            | 5 +++++
>>>  include/linux/lsm_hook_defs.h       | 2 ++
>>>  include/linux/lsm_hooks.h           | 6 ++++++
>>>  include/linux/security.h            | 2 ++
>>>  security/security.c                 | 6 ++++++
>>>  security/selinux/hooks.c            | 9 +++++++++
>>>  security/selinux/include/classmap.h | 2 +-
>>>  7 files changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>>> index 9830848c8d25..9063db04826d 100644
>>> --- a/drivers/android/binder.c
>>> +++ b/drivers/android/binder.c
>>> @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
>>>       if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
>>>               struct dma_buf *dmabuf;
>>>
>>> +             if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
>>> +                     ret = -EPERM;
>>> +                     goto err_security;
>>> +             }
>>> +
>>>               if (unlikely(!is_dma_buf_file(file))) {
>>>                       binder_user_error(
>>>                               "%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>> index ed6cb2ac55fa..8db2a958557e 100644
>>> --- a/include/linux/lsm_hook_defs.h
>>> +++ b/include/linux/lsm_hook_defs.h
>>> @@ -33,6 +33,8 @@ LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
>>>        const struct cred *to)
>>>  LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
>>>        const struct cred *to, struct file *file)
>>> +LSM_HOOK(int, 0, binder_transfer_charge, const struct cred *from,
>>> +      const struct cred *to)
>>>  LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
>>>        unsigned int mode)
>>>  LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index 0a5ba81f7367..39c40c7bf519 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1385,6 +1385,12 @@
>>>   *   @file contains the struct file being transferred.
>>>   *   @to contains the struct cred for the receiving process.
>>>   *   Return 0 if permission is granted.
>>> + * @binder_transfer_charge:
>>> + *   Check whether @from is allowed to transfer the memory charge for a
>>> + *   buffer out of its cgroup to @to.
>>> + *   @from contains the struct cred for the sending process.
>>> + *   @to contains the struct cred for the receiving process.
>>> + *   Return 0 if permission is granted.
>>>   *
>>>   * @ptrace_access_check:
>>>   *   Check permission before allowing the current process to trace the
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 5b67f208f7de..3b7472308430 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -270,6 +270,8 @@ int security_binder_transfer_binder(const struct cred *from,
>>>                                   const struct cred *to);
>>>  int security_binder_transfer_file(const struct cred *from,
>>>                                 const struct cred *to, struct file *file);
>>> +int security_binder_transfer_charge(const struct cred *from,
>>> +                                 const struct cred *to);
>>>  int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
>>>  int security_ptrace_traceme(struct task_struct *parent);
>>>  int security_capget(struct task_struct *target,
>>> diff --git a/security/security.c b/security/security.c
>>> index d1571900a8c7..97e1e74d1ff2 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -801,6 +801,12 @@ int security_binder_transfer_file(const struct cred *from,
>>>       return call_int_hook(binder_transfer_file, 0, from, to, file);
>>>  }
>>>
>>> +int security_binder_transfer_charge(const struct cred *from,
>>> +                                 const struct cred *to)
>>> +{
>>> +     return call_int_hook(binder_transfer_charge, 0, from, to);
>>> +}
>>> +
>>>  int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
>>>  {
>>>       return call_int_hook(ptrace_access_check, 0, child, mode);
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 3c5be76a9199..823ef14924bd 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
>>>                           &ad);
>>>  }
>>>
>>> +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
>>> +{
>>> +     return avc_has_perm(&selinux_state,
>>> +                         cred_sid(from), cred_sid(to),
>>> +                         SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
>>> +                         NULL);
>>> +}
>>> +
>>>  static int selinux_ptrace_access_check(struct task_struct *child,
>>>                                      unsigned int mode)
>>>  {
>>> @@ -7052,6 +7060,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>>       LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>>>       LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
>>>       LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
>>> +     LSM_HOOK_INIT(binder_transfer_charge, selinux_binder_transfer_charge),
>>>
>>>       LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
>>>       LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
>>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>>> index a3c380775d41..2eef180d10d7 100644
>>> --- a/security/selinux/include/classmap.h
>>> +++ b/security/selinux/include/classmap.h
>>> @@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
>>>       { "tun_socket",
>>>         { COMMON_SOCK_PERMS, "attach_queue", NULL } },
>>>       { "binder", { "impersonate", "call", "set_context_mgr", "transfer",
>>> -                   NULL } },
>>> +                   "transfer_charge", NULL } },
>>>       { "cap_userns",
>>>         { COMMON_CAP_PERMS, NULL } },
>>>       { "cap2_userns",

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[T.J. Mercier]

On Tue, Jan 10, 2023 at 11:39 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 1/9/2023 4:30 PM, T.J. Mercier wrote:
> > On Mon, Jan 9, 2023 at 2:28 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 1/9/2023 1:38 PM, T.J. Mercier wrote:
> >>> Any process can cause a memory charge transfer to occur to any other
> >>> process when transmitting a file descriptor through binder. This should
> >>> only be possible for central allocator processes,
> >> How is a "central allocator process" identified?
> > Any process with the transfer_charge permission. On Android this is
> > the graphics allocator HAL which would have this added to its policy.
>
> OK. You're putting SELinux policy directly into the name of the LSM hook.
>
> >
> >> If I have a LSM that
> >> is not SELinux (e.g. AppArmor, Smack) or no LSM at all, how can/should this
> >> be enforced?
> > Sorry, why would you be expecting enforcement with no LSM?
>
> Because the LSM is supposed to be a set of *additional* restrictions.
> If there are no restrictions when there's no LSM, you can't add to
> existing restrictions. If binder works correctly without any restrictions
> that's fine. It seems odd that you'd add SELinux restrictions if there
> are no basic restrictions. If, on the other hand, binder doesn't have
> native restrictions because it always assumes SELinux, it ought to have
> a CONFIG dependency on SELinux.
>
> None of which is really important.
>
> >  Are you
> > suggesting that this check should be different than the ones that
> > already exist for Binder here?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/lsm_hook_defs.h#n29
>
> This one seems just a little bit more like an implementation of
> SELinux policy in the hook than some of the others. If there is no way
> to identify the special process without SELinux policy it's going to
> be really difficult for a different LSM to utilize the hook.
>
I think I see what you're saying... there is potentially no way to
have an equivalent attribute with other LSMs?

> >
> >> Why isn't binder enforcing this restriction itself?
> > Binder has no direct knowledge of which process has been designated as
> > an allocator / charge transferrer. That is defined externally by
> > whoever configures the system.
>
> So the attribute isn't a binder attribute, it's an SELinux attribute?
> That isn't appropriate in the LSM interface, at least not explicitly.
>
The transfer feature is something offered by binder, but mapped to a
process only in the SELinux policy.

> >
> >>>  so a new SELinux
> >>> permission is added to restrict which processes are allowed to initiate
> >>> these charge transfers.
> >> Which is all perfectly reasonable if you have SELinux.
> >>
> >>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> >>> ---
> >>>  drivers/android/binder.c            | 5 +++++
> >>>  include/linux/lsm_hook_defs.h       | 2 ++
> >>>  include/linux/lsm_hooks.h           | 6 ++++++
> >>>  include/linux/security.h            | 2 ++
> >>>  security/security.c                 | 6 ++++++
> >>>  security/selinux/hooks.c            | 9 +++++++++
> >>>  security/selinux/include/classmap.h | 2 +-
> >>>  7 files changed, 31 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >>> index 9830848c8d25..9063db04826d 100644
> >>> --- a/drivers/android/binder.c
> >>> +++ b/drivers/android/binder.c
> >>> @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> >>>       if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> >>>               struct dma_buf *dmabuf;
> >>>
> >>> +             if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> >>> +                     ret = -EPERM;
> >>> +                     goto err_security;
> >>> +             }
> >>> +
> >>>               if (unlikely(!is_dma_buf_file(file))) {
> >>>                       binder_user_error(
> >>>                               "%d:%d got transaction with XFER_CHARGE for non-dmabuf fd, %d\n",
> >>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> >>> index ed6cb2ac55fa..8db2a958557e 100644
> >>> --- a/include/linux/lsm_hook_defs.h
> >>> +++ b/include/linux/lsm_hook_defs.h
> >>> @@ -33,6 +33,8 @@ LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
> >>>        const struct cred *to)
> >>>  LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
> >>>        const struct cred *to, struct file *file)
> >>> +LSM_HOOK(int, 0, binder_transfer_charge, const struct cred *from,
> >>> +      const struct cred *to)
> >>>  LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
> >>>        unsigned int mode)
> >>>  LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)
> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> >>> index 0a5ba81f7367..39c40c7bf519 100644
> >>> --- a/include/linux/lsm_hooks.h
> >>> +++ b/include/linux/lsm_hooks.h
> >>> @@ -1385,6 +1385,12 @@
> >>>   *   @file contains the struct file being transferred.
> >>>   *   @to contains the struct cred for the receiving process.
> >>>   *   Return 0 if permission is granted.
> >>> + * @binder_transfer_charge:
> >>> + *   Check whether @from is allowed to transfer the memory charge for a
> >>> + *   buffer out of its cgroup to @to.
> >>> + *   @from contains the struct cred for the sending process.
> >>> + *   @to contains the struct cred for the receiving process.
> >>> + *   Return 0 if permission is granted.
> >>>   *
> >>>   * @ptrace_access_check:
> >>>   *   Check permission before allowing the current process to trace the
> >>> diff --git a/include/linux/security.h b/include/linux/security.h
> >>> index 5b67f208f7de..3b7472308430 100644
> >>> --- a/include/linux/security.h
> >>> +++ b/include/linux/security.h
> >>> @@ -270,6 +270,8 @@ int security_binder_transfer_binder(const struct cred *from,
> >>>                                   const struct cred *to);
> >>>  int security_binder_transfer_file(const struct cred *from,
> >>>                                 const struct cred *to, struct file *file);
> >>> +int security_binder_transfer_charge(const struct cred *from,
> >>> +                                 const struct cred *to);
> >>>  int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
> >>>  int security_ptrace_traceme(struct task_struct *parent);
> >>>  int security_capget(struct task_struct *target,
> >>> diff --git a/security/security.c b/security/security.c
> >>> index d1571900a8c7..97e1e74d1ff2 100644
> >>> --- a/security/security.c
> >>> +++ b/security/security.c
> >>> @@ -801,6 +801,12 @@ int security_binder_transfer_file(const struct cred *from,
> >>>       return call_int_hook(binder_transfer_file, 0, from, to, file);
> >>>  }
> >>>
> >>> +int security_binder_transfer_charge(const struct cred *from,
> >>> +                                 const struct cred *to)
> >>> +{
> >>> +     return call_int_hook(binder_transfer_charge, 0, from, to);
> >>> +}
> >>> +
> >>>  int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
> >>>  {
> >>>       return call_int_hook(ptrace_access_check, 0, child, mode);
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index 3c5be76a9199..823ef14924bd 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
> >>>                           &ad);
> >>>  }
> >>>
> >>> +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> >>> +{
> >>> +     return avc_has_perm(&selinux_state,
> >>> +                         cred_sid(from), cred_sid(to),
> >>> +                         SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> >>> +                         NULL);
> >>> +}
> >>> +
> >>>  static int selinux_ptrace_access_check(struct task_struct *child,
> >>>                                      unsigned int mode)
> >>>  {
> >>> @@ -7052,6 +7060,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> >>>       LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> >>>       LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
> >>>       LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
> >>> +     LSM_HOOK_INIT(binder_transfer_charge, selinux_binder_transfer_charge),
> >>>
> >>>       LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
> >>>       LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
> >>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> >>> index a3c380775d41..2eef180d10d7 100644
> >>> --- a/security/selinux/include/classmap.h
> >>> +++ b/security/selinux/include/classmap.h
> >>> @@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
> >>>       { "tun_socket",
> >>>         { COMMON_SOCK_PERMS, "attach_queue", NULL } },
> >>>       { "binder", { "impersonate", "call", "set_context_mgr", "transfer",
> >>> -                   NULL } },
> >>> +                   "transfer_charge", NULL } },
> >>>       { "cap_userns",
> >>>         { COMMON_CAP_PERMS, NULL } },
> >>>       { "cap2_userns",

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[Paul Moore]

On Mon, Jan 9, 2023 at 4:38 PM T.J. Mercier <tjmercier@google.com> wrote:
>
> Any process can cause a memory charge transfer to occur to any other
> process when transmitting a file descriptor through binder. This should
> only be possible for central allocator processes, so a new SELinux
> permission is added to restrict which processes are allowed to initiate
> these charge transfers.
>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> ---
>  drivers/android/binder.c            | 5 +++++
>  include/linux/lsm_hook_defs.h       | 2 ++
>  include/linux/lsm_hooks.h           | 6 ++++++
>  include/linux/security.h            | 2 ++
>  security/security.c                 | 6 ++++++
>  security/selinux/hooks.c            | 9 +++++++++
>  security/selinux/include/classmap.h | 2 +-
>  7 files changed, 31 insertions(+), 1 deletion(-)

Hi T.J.,

A few things come to mind when looking at this patchset, but let me
start with the big one first: you only sent 0/4 and 4/4 to the LSM and
SELinux lists, so that's all I'm seeing in my inbox to review, and
it's hard to make sense of what you want to do with just these
snippets.  This makes me cranky, and less inclined to spend the time
to give this a proper review, because there are plenty of other things
which need attention and don't require me having to hunt down missing
pieces.  Yes, I'm aware of b4/lei, and while they are great tools, my
workflow was pretty well established before they came into existence
and I still do things the good ol' fashioned way with mailing lists,
etc.

Make the patch reviewer's life easy whenever you can, it will rarely
(ever?) backfire, I promise.

> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 9830848c8d25..9063db04826d 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
>         if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
>                 struct dma_buf *dmabuf;
>
> +               if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> +                       ret = -EPERM;
> +                       goto err_security;
> +               }

This is where I believe I'm missing the proper context, as this
version of binder_translate_fd() differs from what I see in Linus'
tree.  However, the version in Linus' tree does have a LSM hook,
security_binder_transfer_file(), which is passed both the credentials
you are using above and based solely on the level of indentation shown
in the chunk of code above, it seems like the existing hook might be
suitable?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3c5be76a9199..823ef14924bd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
>                             &ad);
>  }
>
> +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> +{
> +       return avc_has_perm(&selinux_state,
> +                           cred_sid(from), cred_sid(to),
> +                           SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> +                           NULL);
> +}

Generally speaking SELinux doesn't really worry about resource
accounting controls so this seems a bit out of place, but perhaps the
larger question is do you see this being sufficiently distinct from
the existing binder:transfer permission?  In other words, would you
ever want to grant a domain the ability to transfer a file *without*
also granting it the ability to transfer the memory charge?  You need
to help me explain why we need an additional permission for this,
because I don't currently see the need.

>  static int selinux_ptrace_access_check(struct task_struct *child,
>                                        unsigned int mode)
>  {
> @@ -7052,6 +7060,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>         LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
>         LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
> +       LSM_HOOK_INIT(binder_transfer_charge, selinux_binder_transfer_charge),
>
>         LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
>         LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index a3c380775d41..2eef180d10d7 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
>         { "tun_socket",
>           { COMMON_SOCK_PERMS, "attach_queue", NULL } },
>         { "binder", { "impersonate", "call", "set_context_mgr", "transfer",
> -                     NULL } },
> +                     "transfer_charge", NULL } },
>         { "cap_userns",
>           { COMMON_CAP_PERMS, NULL } },
>         { "cap2_userns",
> --
> 2.39.0.314.g84b9a713c41-goog

--
paul-moore.com

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[T.J. Mercier]

On Wed, Jan 11, 2023 at 3:00 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Jan 9, 2023 at 4:38 PM T.J. Mercier <tjmercier@google.com> wrote:
> >
> > Any process can cause a memory charge transfer to occur to any other
> > process when transmitting a file descriptor through binder. This should
> > only be possible for central allocator processes, so a new SELinux
> > permission is added to restrict which processes are allowed to initiate
> > these charge transfers.
> >
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > ---
> >  drivers/android/binder.c            | 5 +++++
> >  include/linux/lsm_hook_defs.h       | 2 ++
> >  include/linux/lsm_hooks.h           | 6 ++++++
> >  include/linux/security.h            | 2 ++
> >  security/security.c                 | 6 ++++++
> >  security/selinux/hooks.c            | 9 +++++++++
> >  security/selinux/include/classmap.h | 2 +-
> >  7 files changed, 31 insertions(+), 1 deletion(-)
>
> Hi T.J.,
>
> A few things come to mind when looking at this patchset, but let me
> start with the big one first: you only sent 0/4 and 4/4 to the LSM and
> SELinux lists, so that's all I'm seeing in my inbox to review, and
> it's hard to make sense of what you want to do with just these
> snippets.  This makes me cranky, and less inclined to spend the time
> to give this a proper review, because there are plenty of other things
> which need attention and don't require me having to hunt down missing
> pieces.  Yes, I'm aware of b4/lei, and while they are great tools, my
> workflow was pretty well established before they came into existence
> and I still do things the good ol' fashioned way with mailing lists,
> etc.
>
> Make the patch reviewer's life easy whenever you can, it will rarely
> (ever?) backfire, I promise.
>
Hi Paul, sorry about that. I have git send-email calling
get_maintainer.pl to automatically figure out the recipients, and I
think that's why it only sent particular patches to a subset of lists.
Looks like the list of recipients for each patch should be a union of
all patches. Thank you for taking a look anyway! Here's a lore link:
https://lore.kernel.org/lkml/20230109213809.418135-1-tjmercier@google.com/

> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 9830848c8d25..9063db04826d 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> >         if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> >                 struct dma_buf *dmabuf;
> >
> > +               if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> > +                       ret = -EPERM;
> > +                       goto err_security;
> > +               }
>
> This is where I believe I'm missing the proper context, as this
> version of binder_translate_fd() differs from what I see in Linus'
> tree.  However, the version in Linus' tree does have a LSM hook,
> security_binder_transfer_file(), which is passed both the credentials
> you are using above and based solely on the level of indentation shown
> in the chunk of code above, it seems like the existing hook might be
> suitable?
>
Yes, patch 3 plumbs through flags to this function:
https://lore.kernel.org/lkml/20230109213809.418135-4-tjmercier@google.com/

I don't think the existing hook is suitable, which I've tried to explain below.

> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3c5be76a9199..823ef14924bd 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
> >                             &ad);
> >  }
> >
> > +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> > +{
> > +       return avc_has_perm(&selinux_state,
> > +                           cred_sid(from), cred_sid(to),
> > +                           SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> > +                           NULL);
> > +}
>
> Generally speaking SELinux doesn't really worry about resource
> accounting controls so this seems a bit out of place, but perhaps the
> larger question is do you see this being sufficiently distinct from
> the existing binder:transfer permission?  In other words, would you
> ever want to grant a domain the ability to transfer a file *without*
> also granting it the ability to transfer the memory charge?  You need
> to help me explain why we need an additional permission for this,
> because I don't currently see the need.
>
Yes, and that's actually more often the case than not. A file here
means a file descriptor that points at any type of resource: file on
disk, memfd, dmabuf, etc. Currently there exists policy that restricts
which processes are allowed to interact with FDs over binder using the
security_binder_transfer_file hook you reference. [1] However this new
transfer_charge permission is meant to restrict the ability of a FD
sender to transfer the memory charge associated with that FD (if one
exists) to a recipient (who may or may not want to accept the memory
charge). So the memory charge is independent of (potentially one-time,
read-only) access to the FD. I thought this would be a good idea for
two reasons:
1) The recipient has no control over whether to accept the memory
charge or not. If the recipient shouldn't have the memory charge
associated with a shared/loaned resource attributed to it, the
recipient doesn't have a way to prevent that. When the charge is
transferred, it could initiate memory reclaim on the recipient, or
make it a bigger target for a future process kill than it should be.
2) Only certain senders should be doing this. Without this control,
any sender could circumvent process memory limits by transferring
memory charges that should be attributed to them onto others.

[1] https://cs.android.com/search?q=%22fd%20use%22&ss=android%2Fplatform%2Fsuperproject:system%2Fsepolicy%2F



> >  static int selinux_ptrace_access_check(struct task_struct *child,
> >                                        unsigned int mode)
> >  {
> > @@ -7052,6 +7060,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> >         LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> >         LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
> >         LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
> > +       LSM_HOOK_INIT(binder_transfer_charge, selinux_binder_transfer_charge),
> >
> >         LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
> >         LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
> > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> > index a3c380775d41..2eef180d10d7 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
> >         { "tun_socket",
> >           { COMMON_SOCK_PERMS, "attach_queue", NULL } },
> >         { "binder", { "impersonate", "call", "set_context_mgr", "transfer",
> > -                     NULL } },
> > +                     "transfer_charge", NULL } },
> >         { "cap_userns",
> >           { COMMON_CAP_PERMS, NULL } },
> >         { "cap2_userns",
> > --
> > 2.39.0.314.g84b9a713c41-goog
>
> --
> paul-moore.com

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[Paul Moore]

On Wed, Jan 11, 2023 at 7:21 PM T.J. Mercier <tjmercier@google.com> wrote:
>
> On Wed, Jan 11, 2023 at 3:00 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Mon, Jan 9, 2023 at 4:38 PM T.J. Mercier <tjmercier@google.com> wrote:
> > >
> > > Any process can cause a memory charge transfer to occur to any other
> > > process when transmitting a file descriptor through binder. This should
> > > only be possible for central allocator processes, so a new SELinux
> > > permission is added to restrict which processes are allowed to initiate
> > > these charge transfers.
> > >
> > > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > > ---
> > >  drivers/android/binder.c            | 5 +++++
> > >  include/linux/lsm_hook_defs.h       | 2 ++
> > >  include/linux/lsm_hooks.h           | 6 ++++++
> > >  include/linux/security.h            | 2 ++
> > >  security/security.c                 | 6 ++++++
> > >  security/selinux/hooks.c            | 9 +++++++++
> > >  security/selinux/include/classmap.h | 2 +-
> > >  7 files changed, 31 insertions(+), 1 deletion(-)
> >
> > Hi T.J.,
> >
> > A few things come to mind when looking at this patchset, but let me
> > start with the big one first: you only sent 0/4 and 4/4 to the LSM and
> > SELinux lists, so that's all I'm seeing in my inbox to review, and
> > it's hard to make sense of what you want to do with just these
> > snippets.  This makes me cranky, and less inclined to spend the time
> > to give this a proper review, because there are plenty of other things
> > which need attention and don't require me having to hunt down missing
> > pieces.  Yes, I'm aware of b4/lei, and while they are great tools, my
> > workflow was pretty well established before they came into existence
> > and I still do things the good ol' fashioned way with mailing lists,
> > etc.
> >
> > Make the patch reviewer's life easy whenever you can, it will rarely
> > (ever?) backfire, I promise.
>
> Hi Paul, sorry about that. I have git send-email calling
> get_maintainer.pl to automatically figure out the recipients, and I
> think that's why it only sent particular patches to a subset of lists.
> Looks like the list of recipients for each patch should be a union of
> all patches. Thank you for taking a look anyway! Here's a lore link:
> https://lore.kernel.org/lkml/20230109213809.418135-1-tjmercier@google.com/
>
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index 9830848c8d25..9063db04826d 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> > >         if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> > >                 struct dma_buf *dmabuf;
> > >
> > > +               if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> > > +                       ret = -EPERM;
> > > +                       goto err_security;
> > > +               }
> >
> > This is where I believe I'm missing the proper context, as this
> > version of binder_translate_fd() differs from what I see in Linus'
> > tree.  However, the version in Linus' tree does have a LSM hook,
> > security_binder_transfer_file(), which is passed both the credentials
> > you are using above and based solely on the level of indentation shown
> > in the chunk of code above, it seems like the existing hook might be
> > suitable?
>
> Yes, patch 3 plumbs through flags to this function:
> https://lore.kernel.org/lkml/20230109213809.418135-4-tjmercier@google.com/
>
> I don't think the existing hook is suitable, which I've tried to explain below.

In this particular case the issue of what permission checks are done
for a given LSM, SELinux in this case, appears to be independent of if
we need a new, different, or second LSM hook.  Unless I missed
something the only real difference with this new hook is that is sits
behind a conditional checking if memory control groups are enabled and
if a transfer charge was specified; it seems like passing the @flags
parameter into the existing LSM hook would allow you to use the
existing hook (it is called before the new hook, right?)?

> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 3c5be76a9199..823ef14924bd 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
> > >                             &ad);
> > >  }
> > >
> > > +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> > > +{
> > > +       return avc_has_perm(&selinux_state,
> > > +                           cred_sid(from), cred_sid(to),
> > > +                           SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> > > +                           NULL);
> > > +}
> >
> > Generally speaking SELinux doesn't really worry about resource
> > accounting controls so this seems a bit out of place, but perhaps the
> > larger question is do you see this being sufficiently distinct from
> > the existing binder:transfer permission?  In other words, would you
> > ever want to grant a domain the ability to transfer a file *without*
> > also granting it the ability to transfer the memory charge?  You need
> > to help me explain why we need an additional permission for this,
> > because I don't currently see the need.
> >
> Yes, and that's actually more often the case than not. A file here
> means a file descriptor that points at any type of resource: file on
> disk, memfd, dmabuf, etc. Currently there exists policy that restricts
> which processes are allowed to interact with FDs over binder using the
> security_binder_transfer_file hook you reference. [1] However this new
> transfer_charge permission is meant to restrict the ability of a FD
> sender to transfer the memory charge associated with that FD (if one
> exists) to a recipient (who may or may not want to accept the memory
> charge). So the memory charge is independent of (potentially one-time,
> read-only) access to the FD.

Without a more comprehensive set of LSM/SELinux access controls around
resource management (which would need discussion beyond just this
thread/patch) I'm not sure we want to start patching in one-off
controls like this.

I haven't looked, but are there any traditional/DAC access controls
around transfering memory changes from one task to another?  It seems
like there *should* be, and if so, it seems like that would be the
right approach at the moment ... if you're not already doing that in
the other patches in the patchset.

-- 
paul-moore.com

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[T.J. Mercier]

On Thu, Jan 12, 2023 at 12:45 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Jan 11, 2023 at 7:21 PM T.J. Mercier <tjmercier@google.com> wrote:
> >
> > On Wed, Jan 11, 2023 at 3:00 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Mon, Jan 9, 2023 at 4:38 PM T.J. Mercier <tjmercier@google.com> wrote:
> > > >
> > > > Any process can cause a memory charge transfer to occur to any other
> > > > process when transmitting a file descriptor through binder. This should
> > > > only be possible for central allocator processes, so a new SELinux
> > > > permission is added to restrict which processes are allowed to initiate
> > > > these charge transfers.
> > > >
> > > > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > > > ---
> > > >  drivers/android/binder.c            | 5 +++++
> > > >  include/linux/lsm_hook_defs.h       | 2 ++
> > > >  include/linux/lsm_hooks.h           | 6 ++++++
> > > >  include/linux/security.h            | 2 ++
> > > >  security/security.c                 | 6 ++++++
> > > >  security/selinux/hooks.c            | 9 +++++++++
> > > >  security/selinux/include/classmap.h | 2 +-
> > > >  7 files changed, 31 insertions(+), 1 deletion(-)
> > >
> > > Hi T.J.,
> > >
> > > A few things come to mind when looking at this patchset, but let me
> > > start with the big one first: you only sent 0/4 and 4/4 to the LSM and
> > > SELinux lists, so that's all I'm seeing in my inbox to review, and
> > > it's hard to make sense of what you want to do with just these
> > > snippets.  This makes me cranky, and less inclined to spend the time
> > > to give this a proper review, because there are plenty of other things
> > > which need attention and don't require me having to hunt down missing
> > > pieces.  Yes, I'm aware of b4/lei, and while they are great tools, my
> > > workflow was pretty well established before they came into existence
> > > and I still do things the good ol' fashioned way with mailing lists,
> > > etc.
> > >
> > > Make the patch reviewer's life easy whenever you can, it will rarely
> > > (ever?) backfire, I promise.
> >
> > Hi Paul, sorry about that. I have git send-email calling
> > get_maintainer.pl to automatically figure out the recipients, and I
> > think that's why it only sent particular patches to a subset of lists.
> > Looks like the list of recipients for each patch should be a union of
> > all patches. Thank you for taking a look anyway! Here's a lore link:
> > https://lore.kernel.org/lkml/20230109213809.418135-1-tjmercier@google.com/
> >
> > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > index 9830848c8d25..9063db04826d 100644
> > > > --- a/drivers/android/binder.c
> > > > +++ b/drivers/android/binder.c
> > > > @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> > > >         if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> > > >                 struct dma_buf *dmabuf;
> > > >
> > > > +               if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> > > > +                       ret = -EPERM;
> > > > +                       goto err_security;
> > > > +               }
> > >
> > > This is where I believe I'm missing the proper context, as this
> > > version of binder_translate_fd() differs from what I see in Linus'
> > > tree.  However, the version in Linus' tree does have a LSM hook,
> > > security_binder_transfer_file(), which is passed both the credentials
> > > you are using above and based solely on the level of indentation shown
> > > in the chunk of code above, it seems like the existing hook might be
> > > suitable?
> >
> > Yes, patch 3 plumbs through flags to this function:
> > https://lore.kernel.org/lkml/20230109213809.418135-4-tjmercier@google.com/
> >
> > I don't think the existing hook is suitable, which I've tried to explain below.
>
> In this particular case the issue of what permission checks are done
> for a given LSM, SELinux in this case, appears to be independent of if
> we need a new, different, or second LSM hook.  Unless I missed
> something the only real difference with this new hook is that is sits
> behind a conditional checking if memory control groups are enabled and
> if a transfer charge was specified; it seems like passing the @flags
> parameter into the existing LSM hook would allow you to use the
> existing hook (it is called before the new hook, right?)?
>
Ah yes, that sounds like it would work. Thank you.

> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 3c5be76a9199..823ef14924bd 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
> > > >                             &ad);
> > > >  }
> > > >
> > > > +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> > > > +{
> > > > +       return avc_has_perm(&selinux_state,
> > > > +                           cred_sid(from), cred_sid(to),
> > > > +                           SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> > > > +                           NULL);
> > > > +}
> > >
> > > Generally speaking SELinux doesn't really worry about resource
> > > accounting controls so this seems a bit out of place, but perhaps the
> > > larger question is do you see this being sufficiently distinct from
> > > the existing binder:transfer permission?  In other words, would you
> > > ever want to grant a domain the ability to transfer a file *without*
> > > also granting it the ability to transfer the memory charge?  You need
> > > to help me explain why we need an additional permission for this,
> > > because I don't currently see the need.
> > >
> > Yes, and that's actually more often the case than not. A file here
> > means a file descriptor that points at any type of resource: file on
> > disk, memfd, dmabuf, etc. Currently there exists policy that restricts
> > which processes are allowed to interact with FDs over binder using the
> > security_binder_transfer_file hook you reference. [1] However this new
> > transfer_charge permission is meant to restrict the ability of a FD
> > sender to transfer the memory charge associated with that FD (if one
> > exists) to a recipient (who may or may not want to accept the memory
> > charge). So the memory charge is independent of (potentially one-time,
> > read-only) access to the FD.
>
> Without a more comprehensive set of LSM/SELinux access controls around
> resource management (which would need discussion beyond just this
> thread/patch) I'm not sure we want to start patching in one-off
> controls like this.
>
Understood, I'll try reusing security_binder_transfer_file.

> I haven't looked, but are there any traditional/DAC access controls
> around transfering memory changes from one task to another?  It seems
> like there *should* be, and if so, it seems like that would be the
> right approach at the moment ... if you're not already doing that in
> the other patches in the patchset.
>
I'm not aware of controls associated with individual objects like
dmabufs. While it's not quite the same thing, I do see that support
for charge migration tied to task migration was intentionally dropped
for cgroup2 and is now deprecated for cgroup1 because it's difficult
and expensive. However that seems like a much bigger job than dealing
with the memory backing an individual object when that object is
handed off between processes (the object ownership moves, not the
task).

https://lore.kernel.org/all/20221206171340.139790-4-hannes@cmpxchg.org/


> --
> paul-moore.com

Re: [PATCH 4/4] security: binder: Add transfer_charge SElinux hook

[Paul Moore]

On Thu, Jan 12, 2023 at 4:36 PM T.J. Mercier <tjmercier@google.com> wrote:
> On Thu, Jan 12, 2023 at 12:45 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Jan 11, 2023 at 7:21 PM T.J. Mercier <tjmercier@google.com> wrote:
> > > On Wed, Jan 11, 2023 at 3:00 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Mon, Jan 9, 2023 at 4:38 PM T.J. Mercier <tjmercier@google.com> wrote:
> > > > >
> > > > > Any process can cause a memory charge transfer to occur to any other
> > > > > process when transmitting a file descriptor through binder. This should
> > > > > only be possible for central allocator processes, so a new SELinux
> > > > > permission is added to restrict which processes are allowed to initiate
> > > > > these charge transfers.
> > > > >
> > > > > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > > > > ---
> > > > >  drivers/android/binder.c            | 5 +++++
> > > > >  include/linux/lsm_hook_defs.h       | 2 ++
> > > > >  include/linux/lsm_hooks.h           | 6 ++++++
> > > > >  include/linux/security.h            | 2 ++
> > > > >  security/security.c                 | 6 ++++++
> > > > >  security/selinux/hooks.c            | 9 +++++++++
> > > > >  security/selinux/include/classmap.h | 2 +-
> > > > >  7 files changed, 31 insertions(+), 1 deletion(-)
> > > >
> > > > Hi T.J.,
> > > >
> > > > A few things come to mind when looking at this patchset, but let me
> > > > start with the big one first: you only sent 0/4 and 4/4 to the LSM and
> > > > SELinux lists, so that's all I'm seeing in my inbox to review, and
> > > > it's hard to make sense of what you want to do with just these
> > > > snippets.  This makes me cranky, and less inclined to spend the time
> > > > to give this a proper review, because there are plenty of other things
> > > > which need attention and don't require me having to hunt down missing
> > > > pieces.  Yes, I'm aware of b4/lei, and while they are great tools, my
> > > > workflow was pretty well established before they came into existence
> > > > and I still do things the good ol' fashioned way with mailing lists,
> > > > etc.
> > > >
> > > > Make the patch reviewer's life easy whenever you can, it will rarely
> > > > (ever?) backfire, I promise.
> > >
> > > Hi Paul, sorry about that. I have git send-email calling
> > > get_maintainer.pl to automatically figure out the recipients, and I
> > > think that's why it only sent particular patches to a subset of lists.
> > > Looks like the list of recipients for each patch should be a union of
> > > all patches. Thank you for taking a look anyway! Here's a lore link:
> > > https://lore.kernel.org/lkml/20230109213809.418135-1-tjmercier@google.com/
> > >
> > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > index 9830848c8d25..9063db04826d 100644
> > > > > --- a/drivers/android/binder.c
> > > > > +++ b/drivers/android/binder.c
> > > > > @@ -2279,6 +2279,11 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, __u32 flags,
> > > > >         if (IS_ENABLED(CONFIG_MEMCG) && (flags & BINDER_FD_FLAG_XFER_CHARGE)) {
> > > > >                 struct dma_buf *dmabuf;
> > > > >
> > > > > +               if (security_binder_transfer_charge(proc->cred, target_proc->cred)) {
> > > > > +                       ret = -EPERM;
> > > > > +                       goto err_security;
> > > > > +               }
> > > >
> > > > This is where I believe I'm missing the proper context, as this
> > > > version of binder_translate_fd() differs from what I see in Linus'
> > > > tree.  However, the version in Linus' tree does have a LSM hook,
> > > > security_binder_transfer_file(), which is passed both the credentials
> > > > you are using above and based solely on the level of indentation shown
> > > > in the chunk of code above, it seems like the existing hook might be
> > > > suitable?
> > >
> > > Yes, patch 3 plumbs through flags to this function:
> > > https://lore.kernel.org/lkml/20230109213809.418135-4-tjmercier@google.com/
> > >
> > > I don't think the existing hook is suitable, which I've tried to explain below.
> >
> > In this particular case the issue of what permission checks are done
> > for a given LSM, SELinux in this case, appears to be independent of if
> > we need a new, different, or second LSM hook.  Unless I missed
> > something the only real difference with this new hook is that is sits
> > behind a conditional checking if memory control groups are enabled and
> > if a transfer charge was specified; it seems like passing the @flags
> > parameter into the existing LSM hook would allow you to use the
> > existing hook (it is called before the new hook, right?)?
> >
> Ah yes, that sounds like it would work. Thank you.
>
> > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > index 3c5be76a9199..823ef14924bd 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -2066,6 +2066,14 @@ static int selinux_binder_transfer_file(const struct cred *from,
> > > > >                             &ad);
> > > > >  }
> > > > >
> > > > > +static int selinux_binder_transfer_charge(const struct cred *from, const struct cred *to)
> > > > > +{
> > > > > +       return avc_has_perm(&selinux_state,
> > > > > +                           cred_sid(from), cred_sid(to),
> > > > > +                           SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
> > > > > +                           NULL);
> > > > > +}
> > > >
> > > > Generally speaking SELinux doesn't really worry about resource
> > > > accounting controls so this seems a bit out of place, but perhaps the
> > > > larger question is do you see this being sufficiently distinct from
> > > > the existing binder:transfer permission?  In other words, would you
> > > > ever want to grant a domain the ability to transfer a file *without*
> > > > also granting it the ability to transfer the memory charge?  You need
> > > > to help me explain why we need an additional permission for this,
> > > > because I don't currently see the need.
> > > >
> > > Yes, and that's actually more often the case than not. A file here
> > > means a file descriptor that points at any type of resource: file on
> > > disk, memfd, dmabuf, etc. Currently there exists policy that restricts
> > > which processes are allowed to interact with FDs over binder using the
> > > security_binder_transfer_file hook you reference. [1] However this new
> > > transfer_charge permission is meant to restrict the ability of a FD
> > > sender to transfer the memory charge associated with that FD (if one
> > > exists) to a recipient (who may or may not want to accept the memory
> > > charge). So the memory charge is independent of (potentially one-time,
> > > read-only) access to the FD.
> >
> > Without a more comprehensive set of LSM/SELinux access controls around
> > resource management (which would need discussion beyond just this
> > thread/patch) I'm not sure we want to start patching in one-off
> > controls like this.
>
> Understood, I'll try reusing security_binder_transfer_file.

Well, yes, it looks like you should reuse the existing hook, but that
is missing the point here - I'm not convinced we want this new SELinux
control without some additional discussion and work around the
desirability and practicality of adding SELinux controls to resource
management.

> > I haven't looked, but are there any traditional/DAC access controls
> > around transfering memory changes from one task to another?  It seems
> > like there *should* be, and if so, it seems like that would be the
> > right approach at the moment ... if you're not already doing that in
> > the other patches in the patchset.
>
> I'm not aware of controls associated with individual objects like
> dmabufs. While it's not quite the same thing, I do see that support
> for charge migration tied to task migration was intentionally dropped
> for cgroup2 and is now deprecated for cgroup1 because it's difficult
> and expensive. However that seems like a much bigger job than dealing
> with the memory backing an individual object when that object is
> handed off between processes (the object ownership moves, not the
> task).

I would suggest you should start talking to the cgroup folks about
this, that seems like a much better first step than jumping straight
to a LSM hook to control just one type of charge transfer.  At the
very least, that is the start of the much larger conversation that
would be needed to add resource management controls to SELinux (and
other LSMs that wanted to implement similar controls).

-- 
paul-moore.com

Re: [PATCH 0/4] Track exported dma-buffers with memcg

[Shakeel Butt]

Hi T.J.,

On Mon, Jan 9, 2023 at 1:38 PM T.J. Mercier <tjmercier@google.com> wrote:
>
> Based on discussions at LPC, this series adds a memory.stat counter for
> exported dmabufs. This counter allows us to continue tracking
> system-wide total exported buffer sizes which there is no longer any
> way to get without DMABUF_SYSFS_STATS, and adds a new capability to
> track per-cgroup exported buffer sizes. The total (root counter) is
> helpful for accounting in-kernel dmabuf use (by comparing with the sum
> of child nodes or with the sum of sizes of mapped buffers or FD
> references in procfs) in addition to helping identify driver memory
> leaks when in-kernel use continually increases over time. With
> per-application cgroups, the per-cgroup counter allows us to quickly
> see how much dma-buf memory an application has caused to be allocated.
> This avoids the need to read through all of procfs which can be a
> lengthy process, and causes the charge to "stick" to the allocating
> process/cgroup as long as the buffer is alive, regardless of how the
> buffer is shared (unless the charge is transferred).
>
> The first patch adds the counter to memcg. The next two patches allow
> the charge for a buffer to be transferred across cgroups which is
> necessary because of the way most dmabufs are allocated from a central
> process on Android. The fourth patch adds a SELinux hook to binder in
> order to control who is allowed to transfer buffer charges.
>
> [1] https://lore.kernel.org/all/20220617085702.4298-1-christian.koenig@amd.com/
>

I am a bit confused by the term "charge" used in this patch series.
From the patches, it seems like only a memcg stat is added and nothing
is charged to the memcg.

This leads me to the question: Why add this stat in memcg if the
underlying memory is not charged to the memcg and if we don't really
want to limit the usage?

I see two ways forward:

1. Instead of memcg, use bpf-rstat [1] infra to implement the
per-cgroup stat for dmabuf. (You may need an additional hook for the
stat transfer).

2. Charge the actual memory to the memcg. Since the size of dmabuf is
immutable across its lifetime, you will not need to do accounting at
page level and instead use something similar to the network memory
accounting interface/mechanism (or even more simple). However you
would need to handle the reclaim, OOM and charge context and failure
cases. However if you are not looking to limit the usage of dmabuf
then this option is an overkill.

Please let me know if I misunderstood something.

[1] https://lore.kernel.org/all/20220824233117.1312810-1-haoluo@google.com/

thanks,
Shakeel

Re: [PATCH 0/4] Track exported dma-buffers with memcg

[Daniel Vetter]

On Mon, Jan 09, 2023 at 04:18:12PM -0800, Shakeel Butt wrote:
> Hi T.J.,
> 
> On Mon, Jan 9, 2023 at 1:38 PM T.J. Mercier <tjmercier@google.com> wrote:
> >
> > Based on discussions at LPC, this series adds a memory.stat counter for
> > exported dmabufs. This counter allows us to continue tracking
> > system-wide total exported buffer sizes which there is no longer any
> > way to get without DMABUF_SYSFS_STATS, and adds a new capability to
> > track per-cgroup exported buffer sizes. The total (root counter) is
> > helpful for accounting in-kernel dmabuf use (by comparing with the sum
> > of child nodes or with the sum of sizes of mapped buffers or FD
> > references in procfs) in addition to helping identify driver memory
> > leaks when in-kernel use continually increases over time. With
> > per-application cgroups, the per-cgroup counter allows us to quickly
> > see how much dma-buf memory an application has caused to be allocated.
> > This avoids the need to read through all of procfs which can be a
> > lengthy process, and causes the charge to "stick" to the allocating
> > process/cgroup as long as the buffer is alive, regardless of how the
> > buffer is shared (unless the charge is transferred).
> >
> > The first patch adds the counter to memcg. The next two patches allow
> > the charge for a buffer to be transferred across cgroups which is
> > necessary because of the way most dmabufs are allocated from a central
> > process on Android. The fourth patch adds a SELinux hook to binder in
> > order to control who is allowed to transfer buffer charges.
> >
> > [1] https://lore.kernel.org/all/20220617085702.4298-1-christian.koenig@amd.com/
> >
> 
> I am a bit confused by the term "charge" used in this patch series.
> From the patches, it seems like only a memcg stat is added and nothing
> is charged to the memcg.
> 
> This leads me to the question: Why add this stat in memcg if the
> underlying memory is not charged to the memcg and if we don't really
> want to limit the usage?
> 
> I see two ways forward:
> 
> 1. Instead of memcg, use bpf-rstat [1] infra to implement the
> per-cgroup stat for dmabuf. (You may need an additional hook for the
> stat transfer).
> 
> 2. Charge the actual memory to the memcg. Since the size of dmabuf is
> immutable across its lifetime, you will not need to do accounting at
> page level and instead use something similar to the network memory
> accounting interface/mechanism (or even more simple). However you
> would need to handle the reclaim, OOM and charge context and failure
> cases. However if you are not looking to limit the usage of dmabuf
> then this option is an overkill.

I think eventually, at least for other "account gpu stuff in cgroups" use
case we do want to actually charge the memory.

The problem is a bit that with gpu allocations reclaim is essentially "we
pass the error to userspace and they get to sort the mess out". There are
some exceptions (some gpu drivers to have shrinkers) would we need to make
sure these shrinkers are tied into the cgroup stuff before we could enable
charging for them?

Also note that at least from the gpu driver side this is all a huge
endeavour, so if we can split up the steps as much as possible (and get
something interim useable that doesn't break stuff ofc), that is
practically need to make headway here. TJ has been trying out various
approaches for quite some time now already :-/
-Daniel

> Please let me know if I misunderstood something.
> 
> [1] https://lore.kernel.org/all/20220824233117.1312810-1-haoluo@google.com/
> 
> thanks,
> Shakeel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] Track exported dma-buffers with memcg

[T.J. Mercier]

On Wed, Jan 11, 2023 at 2:56 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jan 09, 2023 at 04:18:12PM -0800, Shakeel Butt wrote:
> > Hi T.J.,
> >
> > On Mon, Jan 9, 2023 at 1:38 PM T.J. Mercier <tjmercier@google.com> wrote:
> > >
> > > Based on discussions at LPC, this series adds a memory.stat counter for
> > > exported dmabufs. This counter allows us to continue tracking
> > > system-wide total exported buffer sizes which there is no longer any
> > > way to get without DMABUF_SYSFS_STATS, and adds a new capability to
> > > track per-cgroup exported buffer sizes. The total (root counter) is
> > > helpful for accounting in-kernel dmabuf use (by comparing with the sum
> > > of child nodes or with the sum of sizes of mapped buffers or FD
> > > references in procfs) in addition to helping identify driver memory
> > > leaks when in-kernel use continually increases over time. With
> > > per-application cgroups, the per-cgroup counter allows us to quickly
> > > see how much dma-buf memory an application has caused to be allocated.
> > > This avoids the need to read through all of procfs which can be a
> > > lengthy process, and causes the charge to "stick" to the allocating
> > > process/cgroup as long as the buffer is alive, regardless of how the
> > > buffer is shared (unless the charge is transferred).
> > >
> > > The first patch adds the counter to memcg. The next two patches allow
> > > the charge for a buffer to be transferred across cgroups which is
> > > necessary because of the way most dmabufs are allocated from a central
> > > process on Android. The fourth patch adds a SELinux hook to binder in
> > > order to control who is allowed to transfer buffer charges.
> > >
> > > [1] https://lore.kernel.org/all/20220617085702.4298-1-christian.koenig@amd.com/
> > >
> >
> > I am a bit confused by the term "charge" used in this patch series.
> > From the patches, it seems like only a memcg stat is added and nothing
> > is charged to the memcg.
> >
> > This leads me to the question: Why add this stat in memcg if the
> > underlying memory is not charged to the memcg and if we don't really
> > want to limit the usage?
> >
> > I see two ways forward:
> >
> > 1. Instead of memcg, use bpf-rstat [1] infra to implement the
> > per-cgroup stat for dmabuf. (You may need an additional hook for the
> > stat transfer).
> >
> > 2. Charge the actual memory to the memcg. Since the size of dmabuf is
> > immutable across its lifetime, you will not need to do accounting at
> > page level and instead use something similar to the network memory
> > accounting interface/mechanism (or even more simple). However you
> > would need to handle the reclaim, OOM and charge context and failure
> > cases. However if you are not looking to limit the usage of dmabuf
> > then this option is an overkill.
>
> I think eventually, at least for other "account gpu stuff in cgroups" use
> case we do want to actually charge the memory.
>
Yes, I've been looking at this today.

> The problem is a bit that with gpu allocations reclaim is essentially "we
> pass the error to userspace and they get to sort the mess out". There are
> some exceptions (some gpu drivers to have shrinkers) would we need to make
> sure these shrinkers are tied into the cgroup stuff before we could enable
> charging for them?
>
I'm also not sure that we can depend on the dmabuf being backed at
export time 100% of the time? (They are for dmabuf heaps.) If not,
that'd make calling the existing memcg folio based functions a bit
difficult.

> Also note that at least from the gpu driver side this is all a huge
> endeavour, so if we can split up the steps as much as possible (and get
> something interim useable that doesn't break stuff ofc), that is
> practically need to make headway here. TJ has been trying out various
> approaches for quite some time now already :-/
> -Daniel
>
> > Please let me know if I misunderstood something.
> >
> > [1] https://lore.kernel.org/all/20220824233117.1312810-1-haoluo@google.com/
> >
> > thanks,
> > Shakeel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/4] Track exported dma-buffers with memcg

[Shakeel Butt]

On Wed, Jan 11, 2023 at 04:49:36PM -0800, T.J. Mercier wrote:
> 
[...]
> > The problem is a bit that with gpu allocations reclaim is essentially "we
> > pass the error to userspace and they get to sort the mess out". There are
> > some exceptions (some gpu drivers to have shrinkers) would we need to make
> > sure these shrinkers are tied into the cgroup stuff before we could enable
> > charging for them?
> >
> I'm also not sure that we can depend on the dmabuf being backed at
> export time 100% of the time? (They are for dmabuf heaps.) If not,
> that'd make calling the existing memcg folio based functions a bit
> difficult.
> 

Where does the actual memory get allocated? I see the first patch is
updating the stat in dma_buf_export() and dma_buf_release(). Does the
memory get allocated and freed in those code paths?

Re: [PATCH 0/4] Track exported dma-buffers with memcg

[Christian König]

Am 12.01.23 um 09:13 schrieb Shakeel Butt:
> On Wed, Jan 11, 2023 at 04:49:36PM -0800, T.J. Mercier wrote:
> [...]
>>> The problem is a bit that with gpu allocations reclaim is essentially "we
>>> pass the error to userspace and they get to sort the mess out". There are
>>> some exceptions (some gpu drivers to have shrinkers) would we need to make
>>> sure these shrinkers are tied into the cgroup stuff before we could enable
>>> charging for them?
>>>
>> I'm also not sure that we can depend on the dmabuf being backed at
>> export time 100% of the time? (They are for dmabuf heaps.) If not,
>> that'd make calling the existing memcg folio based functions a bit
>> difficult.
>>
> Where does the actual memory get allocated? I see the first patch is
> updating the stat in dma_buf_export() and dma_buf_release(). Does the
> memory get allocated and freed in those code paths?

Nope, dma_buf_export() just makes the memory available to others.

The driver which calls dma_buf_export() is the one allocating the memory.

Regards,
Christian.

Re: [PATCH 0/4] Track exported dma-buffers with memcg

[Shakeel Butt]

On Wed, Jan 11, 2023 at 11:56:45PM +0100, Daniel Vetter wrote:
> 
[...]
> I think eventually, at least for other "account gpu stuff in cgroups" use
> case we do want to actually charge the memory.
> 
> The problem is a bit that with gpu allocations reclaim is essentially "we
> pass the error to userspace and they get to sort the mess out". There are
> some exceptions (some gpu drivers to have shrinkers) would we need to make
> sure these shrinkers are tied into the cgroup stuff before we could enable
> charging for them?
> 

No, there is no requirement to have shrinkers or making such memory
reclaimable before charging it. Though existing shrinkers and the
possible future shrinkers would need to be converted into memcg aware
shrinkers.

Though there will be a need to update user expectations that if they 
use memcgs with hard limits, they may start seeing memcg OOMs after the
charging of dmabuf.

> Also note that at least from the gpu driver side this is all a huge
> endeavour, so if we can split up the steps as much as possible (and get
> something interim useable that doesn't break stuff ofc), that is
> practically need to make headway here. 

This sounds reasonable to me.

Re: [PATCH 0/4] Track exported dma-buffers with memcg

[Michal Hocko]

On Thu 12-01-23 07:56:31, Shakeel Butt wrote:
> On Wed, Jan 11, 2023 at 11:56:45PM +0100, Daniel Vetter wrote:
> > 
> [...]
> > I think eventually, at least for other "account gpu stuff in cgroups" use
> > case we do want to actually charge the memory.
> > 
> > The problem is a bit that with gpu allocations reclaim is essentially "we
> > pass the error to userspace and they get to sort the mess out". There are
> > some exceptions (some gpu drivers to have shrinkers) would we need to make
> > sure these shrinkers are tied into the cgroup stuff before we could enable
> > charging for them?
> > 
> 
> No, there is no requirement to have shrinkers or making such memory
> reclaimable before charging it. Though existing shrinkers and the
> possible future shrinkers would need to be converted into memcg aware
> shrinkers.
> 
> Though there will be a need to update user expectations that if they 
> use memcgs with hard limits, they may start seeing memcg OOMs after the
> charging of dmabuf.

Agreed. This wouldn't be the first in kernel memory charged memory that
is not directly reclaimable. With a dedicated counter an excessive
dmabuf usage would be visible in the oom report because we do print
memcg stats.

It is definitely preferable to have a shrinker mechanism but if that is
to be done in a follow up step then this is acceptable. But leaving out
charging from early on sounds like a bad choice to me.
-- 
Michal Hocko
SUSE Labs