Re: [PATCH 1/3] selinux: make dentry_init_security() return security module name

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 6, 2018 at 11:42 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>
> On 9/6/2018 8:08 AM, Jeff Layton wrote:
> > On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote:
> >> This is preparation for CephFS security label. CephFS's implementation uses
> >> dentry_init_security() to get security context before inode is created,
> >> then sends open/mkdir/mknod request to MDS, together with security xattr
> >> "security.<security module name>"
>
> Please excuse my late entry into this review.
>
> First, *Do not prefix general LSM interface work with "selinux:"*
> You should only use that prefix when you're dealing with the
> internals of SELinux. Use "LSM:" if you are changing LSM interfaces.
> If you are changing security.h or lsm_hooks.h you are almost
> certainly changing LSM interfaces.
>
> Because you prefixed this work with "selinux:" I pretty much
> ignored it. Please don't do that again.
>
> Why aren't you using security_inode_getsecctx, like kernfs and nfs?
>

Sorry, I don't understand why you mention security_inode_getsecctx().
The selinux_dentry_init_security is called for new inode, I don't how
security_inode_getsecctx() can help.

Regards
Yan, Zheng



>
> >>
> >> Signed-off-by: "Yan, Zheng" <zyan@xxxxxxxxxx>
> >> ---
> >>  fs/nfs/nfs4proc.c         | 3 ++-
> >>  include/linux/lsm_hooks.h | 4 ++--
> >>  include/linux/security.h  | 9 +++++----
> >>  security/security.c       | 7 ++++---
> >>  security/selinux/hooks.c  | 8 ++++++--
> >>  5 files changed, 19 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index 6dd146885da9..d18a5fb7aec3 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> >> @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> >>              return NULL;
> >>
> >>      err = security_dentry_init_security(dentry, sattr->ia_mode,
> >> -                            &dentry->d_name, (void **)&label->label, &label->len);
> >> +                            &dentry->d_name,  NULL,
> >> +                            (void **)&label->label, &label->len);
> >>      if (err == 0)
> >>              return label;
> >>
> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> >> index 8f1131c8dd54..e176c2032bdc 100644
> >> --- a/include/linux/lsm_hooks.h
> >> +++ b/include/linux/lsm_hooks.h
> >> @@ -1476,8 +1476,8 @@ union security_list_options {
> >>                                      unsigned long *set_kern_flags);
> >>      int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts);
> >>      int (*dentry_init_security)(struct dentry *dentry, int mode,
> >> -                                    const struct qstr *name, void **ctx,
> >> -                                    u32 *ctxlen);
> >> +                                    const struct qstr *name, const char **label,
> >> +                                    void **ctx, u32 *ctxlen);
> >>      int (*dentry_create_files_as)(struct dentry *dentry, int mode,
> >>                                      struct qstr *name,
> >>                                      const struct cred *old,
> >> diff --git a/include/linux/security.h b/include/linux/security.h
> >> index 63030c85ee19..df2d73998c64 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> >>                              unsigned long *set_kern_flags);
> >>  int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);
> >>  int security_dentry_init_security(struct dentry *dentry, int mode,
> >> -                                    const struct qstr *name, void **ctx,
> >> -                                    u32 *ctxlen);
> >> +                                    const struct qstr *name,
> >> +                                    const char **label,
> >> +                                    void **ctx, u32 *ctxlen);
> >>  int security_dentry_create_files_as(struct dentry *dentry, int mode,
> >>                                      struct qstr *name,
> >>                                      const struct cred *old,
> >> @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode)
> >>  static inline int security_dentry_init_security(struct dentry *dentry,
> >>                                               int mode,
> >>                                               const struct qstr *name,
> >> -                                             void **ctx,
> >> -                                             u32 *ctxlen)
> >> +                                             const char **label,
> >> +                                             void **ctx, u32 *ctxlen)
> >>  {
> >>      return -EOPNOTSUPP;
> >>  }
> >> diff --git a/security/security.c b/security/security.c
> >> index 68f46d849abe..69818d46aa28 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode)
> >>  }
> >>
> >>  int security_dentry_init_security(struct dentry *dentry, int mode,
> >> -                                    const struct qstr *name, void **ctx,
> >> -                                    u32 *ctxlen)
> >> +                                    const struct qstr *name,
> >> +                                    const char **label,
> >> +                                    void **ctx, u32 *ctxlen)
> >>  {
> >>      return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> >> -                            name, ctx, ctxlen);
> >> +                            name, label, ctx, ctxlen);
> >>  }
> >>  EXPORT_SYMBOL(security_dentry_init_security);
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 2b5ee5fbd652..eca3879d9357 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode)
> >>  }
> >>
> >>  static int selinux_dentry_init_security(struct dentry *dentry, int mode,
> >> -                                    const struct qstr *name, void **ctx,
> >> -                                    u32 *ctxlen)
> >> +                                    const struct qstr *name,
> >> +                                    const char **label,
> >> +                                    void **ctx, u32 *ctxlen)
> >>  {
> >>      u32 newsid;
> >>      int rc;
> >> @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
> >>      if (rc)
> >>              return rc;
> >>
> >> +    if (label)
> >> +            *label = XATTR_SELINUX_SUFFIX;
> >> +
> > nit: I'd probably call this "name" since that's what it's called in
> > selinux_inode_init_security. "label" has a different connotation in this
> > context.
> >
> >>      return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
> >>                                     ctxlen);
> >>  }
> > Patch looks reasonable to me though.
> >
> > Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >
> >
>



[Index of Archives]     [CEPH Users]     [Ceph Large]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux