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

From: Paul Moore <paul@paul-moore.com>
To: Joel Granados <j.granados@samsung.com>
Cc: mcgrof@kernel.org, ddiss@suse.de, joshi.k@samsung.com,
	ming.lei@redhat.com, linux-security-module@vger.kernel.org,
	axboe@kernel.dk, io-uring@vger.kernel.org
Subject: Re: [RFC v2 1/1] Use a fs callback to set security specific data
Date: Wed, 23 Nov 2022 16:02:02 -0500	[thread overview]
Message-ID: <CAHC9VhTs26x+6TSbPyQg7y0j=gLCc=_GPgmiUgA34wfxmakvZQ@mail.gmail.com> (raw)
In-Reply-To: <20221122103144.960752-2-j.granados@samsung.com>

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.

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).

>         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);

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

-- 
paul-moore.com