On Wed, Jan 11, 2023 at 7:21 PM T.J. Mercier <tjmercier@xxxxxxxxxx> wrote: > > On Wed, Jan 11, 2023 at 3:00 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > On Mon, Jan 9, 2023 at 4:38 PM T.J. Mercier <tjmercier@xxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx/ > > > > 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@xxxxxxxxxx/ > > 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
