Re: [RFC v2 1/1] Use a fs callback to set security specific data

[Joel Granados <j.granados@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4758 bytes --]

On Wed, Nov 23, 2022 at 04:02:02PM -0500, Paul Moore wrote:
> On Tue, Nov 22, 2022 at 5:35 AM Joel Granados <j.granados@samsung.com> wrote:
> >
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  drivers/nvme/host/core.c      | 10 ++++++++++
> >  include/linux/fs.h            |  2 ++
> >  include/linux/lsm_hook_defs.h |  3 ++-
> >  include/linux/security.h      | 16 ++++++++++++++--
> >  io_uring/uring_cmd.c          |  3 ++-
> >  security/security.c           |  5 +++--
> >  security/selinux/hooks.c      | 16 +++++++++++++++-
> >  7 files changed, 48 insertions(+), 7 deletions(-)
> 
> ...
> 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f553c370397e..9fe3a230c671 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -21,6 +21,8 @@
> >   *  Copyright (C) 2016 Mellanox Technologies
> >   */
> >
> > +#include "linux/nvme_ioctl.h"
> > +#include "linux/security.h"
> >  #include <linux/init.h>
> >  #include <linux/kd.h>
> >  #include <linux/kernel.h>
> > @@ -6999,18 +7001,30 @@ static int selinux_uring_sqpoll(void)
> >   * IORING_OP_URING_CMD against the device/file specified in @ioucmd.
> >   *
> >   */
> > -static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> > +static int selinux_uring_cmd(struct io_uring_cmd *ioucmd,
> > +       int (*uring_cmd_sec)(struct io_uring_cmd *ioucmd, struct security_uring_cmd*))
> >  {
> 
> As we discussed in the previous thread, and Casey mentioned already,
> passing a function pointer for the LSM to call isn't a great practice.
> When it was proposed we hadn't really thought of any alternatives, but
> if we can't find a good scalar value to compare somewhere, I think
> inspecting the file_operations::owner::name string to determine the
> target is preferable to the function pointer approach described here.

We don't only need to determine the target; we need the target to
specify the current operation to the LSM infra so it can do its thing.

To me, if we just identify, we would need to have some logic in the
uring_cmd that goes through all the possible uring users to execute
their security specific functions. (Paul please correct me if I'm
misunderstanding you). Something like this

switch (uring_user_type(req->file->f_op->name)):
case nvme:
  nvme_specific_sec_call();
case ublk:
  ublk_specific_sec_call();
case user3:
  user3_specific_sec_call();
..... etc...

This is not scalable because there would need to be uring user code in
uring and that makes no sense as uring is agnostic to whatever is using
it.

> 
> Although I really would like to see us find, or create, some sort of
> scalar token ID we could use instead.  I fear that doing a lot of
> strcmp()'s to identify the uring command target is going to be a
> problem (one strcmp() for each possible target, multiplied by the
> number of LSMs which implement a io_uring command hook).
Agreed, depending on string compare does not scale.

> 
> >         struct file *file = ioucmd->file;
> >         struct inode *inode = file_inode(file);
> >         struct inode_security_struct *isec = selinux_inode(inode);
> >         struct common_audit_data ad;
> > +       const struct cred *cred = current_cred();
> > +       struct security_uring_cmd sec_uring = {0};
> > +       int ret;
> >
> >         ad.type = LSM_AUDIT_DATA_FILE;
> >         ad.u.file = file;
> >
> > +       ret = uring_cmd_sec(ioucmd, &sec_uring);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (sec_uring.flags & SECURITY_URING_CMD_TYPE_IOCTL)
> > +               return ioctl_has_perm(cred, file, FILE__IOCTL, (u16) ioucmd->cmd_op);
> 
> As mentioned previously, we'll need a SELinux policy capability here
> to preserve the SECCLASS_IO_URING/IO_URING__CMD access check for
> existing users/policies.  I expect the logic would look something like
> this (of course the details are dependent on how we identify the
> target module/device/etc.):
> 
>   if (polcap_foo && uring_tgt) {
>     switch (uring_tgt) {
>     case NVME:
>       return avc_has_perm(...);
>     default:
>       WARN();
>       return avc_has_perm(SECCLASS_IO_URING, IO_URING__CMD);
>     }
>   } else
>     return avc_has_perm(SECCLASS_IO_URING, IO_URING__CMD);
> 
This is selinux specific. right? I ask because I want to have it
strait in my head what is LSM generic and what is needed for a specific
implementation of an LSM.

> >         return avc_has_perm(&selinux_state, current_sid(), isec->sid,
> >                             SECCLASS_IO_URING, IO_URING__CMD, &ad);
> > +
> >  }
> >  #endif /* CONFIG_IO_URING */
> 
> -- 
> paul-moore.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]