* [PATCH] security: Return xattr name from security_dentry_init_security()
@ 2021-09-30 18:59 Vivek Goyal
2021-09-30 20:20 ` Casey Schaufler
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Vivek Goyal @ 2021-09-30 18:59 UTC (permalink / raw)
To: linux-security-module, selinux
Cc: linux-fsdevel, virtio-fs, casey, Miklos Szeredi, Daniel J Walsh,
jlayton, idryomov, ceph-devel, linux-nfs, bfields, chuck.lever,
stephen.smalley.work
Right now security_dentry_init_security() only supports single security
label and is used by SELinux only. There are two users of of this hook,
namely ceph and nfs.
NFS does not care about xattr name. Ceph hardcodes the xattr name to
security.selinux (XATTR_NAME_SELINUX).
I am making changes to fuse/virtiofs to send security label to virtiofsd
and I need to send xattr name as well. I also hardcoded the name of
xattr to security.selinux.
Stephen Smalley suggested that it probably is a good idea to modify
security_dentry_init_security() to also return name of xattr so that
we can avoid this hardcoding in the callers.
This patch adds a new parameter "const char **xattr_name" to
security_dentry_init_security() and LSM puts the name of xattr
too if caller asked for it (xattr_name != NULL).
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
I have compile tested this patch. Don't know how to setup ceph and
test it. Its a very simple change. Hopefully ceph developers can
have a quick look at it.
A similar attempt was made three years back.
https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
---
fs/ceph/xattr.c | 3 +--
fs/nfs/nfs4proc.c | 3 ++-
include/linux/lsm_hook_defs.h | 3 ++-
include/linux/lsm_hooks.h | 1 +
include/linux/security.h | 6 ++++--
security/security.c | 7 ++++---
security/selinux/hooks.c | 6 +++++-
7 files changed, 19 insertions(+), 10 deletions(-)
Index: redhat-linux/security/selinux/hooks.c
===================================================================
--- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
+++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
@@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
}
static int selinux_dentry_init_security(struct dentry *dentry, int mode,
- const struct qstr *name, void **ctx,
+ const struct qstr *name,
+ const char **xattr_name, void **ctx,
u32 *ctxlen)
{
u32 newsid;
@@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
if (rc)
return rc;
+ if (xattr_name)
+ *xattr_name = XATTR_NAME_SELINUX;
+
return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
ctxlen);
}
Index: redhat-linux/security/security.c
===================================================================
--- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
+++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
@@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
}
int security_dentry_init_security(struct dentry *dentry, int mode,
- const struct qstr *name, void **ctx,
- u32 *ctxlen)
+ const struct qstr *name,
+ const char **xattr_name, void **ctx,
+ u32 *ctxlen)
{
return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
- name, ctx, ctxlen);
+ name, xattr_name, ctx, ctxlen);
}
EXPORT_SYMBOL(security_dentry_init_security);
Index: redhat-linux/include/linux/lsm_hooks.h
===================================================================
--- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
+++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
@@ -196,6 +196,7 @@
* @dentry dentry to use in calculating the context.
* @mode mode used to determine resource type.
* @name name of the last path component used to create file
+ * @xattr_name pointer to place the pointer to security xattr name
* @ctx pointer to place the pointer to the resulting context in.
* @ctxlen point to place the length of the resulting context.
* @dentry_create_files_as:
Index: redhat-linux/include/linux/security.h
===================================================================
--- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
+++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
@@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
int len, void **mnt_opts);
int security_move_mount(const struct path *from_path, const struct path *to_path);
int security_dentry_init_security(struct dentry *dentry, int mode,
- const struct qstr *name, void **ctx,
- u32 *ctxlen);
+ const struct qstr *name,
+ const char **xattr_name, void **ctx,
+ u32 *ctxlen);
int security_dentry_create_files_as(struct dentry *dentry, int mode,
struct qstr *name,
const struct cred *old,
@@ -739,6 +740,7 @@ static inline void security_inode_free(s
static inline int security_dentry_init_security(struct dentry *dentry,
int mode,
const struct qstr *name,
+ const char **xattr_name,
void **ctx,
u32 *ctxlen)
{
Index: redhat-linux/include/linux/lsm_hook_defs.h
===================================================================
--- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
+++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
@@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
LSM_HOOK(int, 0, move_mount, const struct path *from_path,
const struct path *to_path)
LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
- int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
+ int mode, const struct qstr *name, const char **xattr_name,
+ void **ctx, u32 *ctxlen)
LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
struct qstr *name, const struct cred *old, struct cred *new)
Index: redhat-linux/fs/nfs/nfs4proc.c
===================================================================
--- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
+++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
@@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
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;
Index: redhat-linux/fs/ceph/xattr.c
===================================================================
--- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
+++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
@@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
int err;
err = security_dentry_init_security(dentry, mode, &dentry->d_name,
- &as_ctx->sec_ctx,
+ &name, &as_ctx->sec_ctx,
&as_ctx->sec_ctxlen);
if (err < 0) {
WARN_ON_ONCE(err != -EOPNOTSUPP);
@@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
* It only supports single security module and only selinux has
* dentry_init_security hook.
*/
- name = XATTR_NAME_SELINUX;
name_len = strlen(name);
err = ceph_pagelist_reserve(pagelist,
4 * 2 + name_len + as_ctx->sec_ctxlen);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-09-30 18:59 [PATCH] security: Return xattr name from security_dentry_init_security() Vivek Goyal
@ 2021-09-30 20:20 ` Casey Schaufler
2021-10-02 18:10 ` Al Viro
2021-10-04 15:20 ` Jeff Layton
2 siblings, 0 replies; 11+ messages in thread
From: Casey Schaufler @ 2021-09-30 20:20 UTC (permalink / raw)
To: Vivek Goyal, linux-security-module, selinux
Cc: linux-fsdevel, virtio-fs, Miklos Szeredi, Daniel J Walsh,
jlayton, idryomov, ceph-devel, linux-nfs, bfields, chuck.lever,
stephen.smalley.work, Casey Schaufler
On 9/30/2021 11:59 AM, Vivek Goyal wrote:
> Right now security_dentry_init_security() only supports single security
> label and is used by SELinux only. There are two users of of this hook,
> namely ceph and nfs.
>
> NFS does not care about xattr name. Ceph hardcodes the xattr name to
> security.selinux (XATTR_NAME_SELINUX).
>
> I am making changes to fuse/virtiofs to send security label to virtiofsd
> and I need to send xattr name as well. I also hardcoded the name of
> xattr to security.selinux.
>
> Stephen Smalley suggested that it probably is a good idea to modify
> security_dentry_init_security() to also return name of xattr so that
> we can avoid this hardcoding in the callers.
>
> This patch adds a new parameter "const char **xattr_name" to
> security_dentry_init_security() and LSM puts the name of xattr
> too if caller asked for it (xattr_name != NULL).
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
This is a reasonable step. It doesn't address the possibility
of multiple security attributes, but that's not a real issue today.
It will prevent ceph from using the wrong attribute should a
security module other than SELinux provide a hook.
> ---
>
> I have compile tested this patch. Don't know how to setup ceph and
> test it. Its a very simple change. Hopefully ceph developers can
> have a quick look at it.
>
> A similar attempt was made three years back.
>
> https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
My, but I dislike reliving my old arguments.
> ---
> fs/ceph/xattr.c | 3 +--
> fs/nfs/nfs4proc.c | 3 ++-
> include/linux/lsm_hook_defs.h | 3 ++-
> include/linux/lsm_hooks.h | 1 +
> include/linux/security.h | 6 ++++--
> security/security.c | 7 ++++---
> security/selinux/hooks.c | 6 +++++-
> 7 files changed, 19 insertions(+), 10 deletions(-)
>
> Index: redhat-linux/security/selinux/hooks.c
> ===================================================================
> --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
> +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
> @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
> }
>
> static int selinux_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> + const struct qstr *name,
> + const char **xattr_name, void **ctx,
> u32 *ctxlen)
> {
> u32 newsid;
> @@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
> if (rc)
> return rc;
>
> + if (xattr_name)
> + *xattr_name = XATTR_NAME_SELINUX;
> +
> return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
> ctxlen);
> }
> Index: redhat-linux/security/security.c
> ===================================================================
> --- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
> +++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
> @@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
> }
>
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen)
> + const struct qstr *name,
> + const char **xattr_name, void **ctx,
> + u32 *ctxlen)
> {
> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> - name, ctx, ctxlen);
> + name, xattr_name, ctx, ctxlen);
> }
> EXPORT_SYMBOL(security_dentry_init_security);
>
> Index: redhat-linux/include/linux/lsm_hooks.h
> ===================================================================
> --- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
> +++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
> @@ -196,6 +196,7 @@
> * @dentry dentry to use in calculating the context.
> * @mode mode used to determine resource type.
> * @name name of the last path component used to create file
> + * @xattr_name pointer to place the pointer to security xattr name
> * @ctx pointer to place the pointer to the resulting context in.
> * @ctxlen point to place the length of the resulting context.
> * @dentry_create_files_as:
> Index: redhat-linux/include/linux/security.h
> ===================================================================
> --- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
> +++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
> @@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
> int len, void **mnt_opts);
> int security_move_mount(const struct path *from_path, const struct path *to_path);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen);
> + const struct qstr *name,
> + const char **xattr_name, void **ctx,
> + u32 *ctxlen);
> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> @@ -739,6 +740,7 @@ static inline void security_inode_free(s
> static inline int security_dentry_init_security(struct dentry *dentry,
> int mode,
> const struct qstr *name,
> + const char **xattr_name,
> void **ctx,
> u32 *ctxlen)
> {
> Index: redhat-linux/include/linux/lsm_hook_defs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
> +++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
> @@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
> LSM_HOOK(int, 0, move_mount, const struct path *from_path,
> const struct path *to_path)
> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> - int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
> + int mode, const struct qstr *name, const char **xattr_name,
> + void **ctx, u32 *ctxlen)
> LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> struct qstr *name, const struct cred *old, struct cred *new)
>
> Index: redhat-linux/fs/nfs/nfs4proc.c
> ===================================================================
> --- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
> +++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
> @@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
> 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;
>
> Index: redhat-linux/fs/ceph/xattr.c
> ===================================================================
> --- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
> +++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
> @@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
> int err;
>
> err = security_dentry_init_security(dentry, mode, &dentry->d_name,
> - &as_ctx->sec_ctx,
> + &name, &as_ctx->sec_ctx,
> &as_ctx->sec_ctxlen);
> if (err < 0) {
> WARN_ON_ONCE(err != -EOPNOTSUPP);
> @@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
> * It only supports single security module and only selinux has
> * dentry_init_security hook.
> */
> - name = XATTR_NAME_SELINUX;
> name_len = strlen(name);
> err = ceph_pagelist_reserve(pagelist,
> 4 * 2 + name_len + as_ctx->sec_ctxlen);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-09-30 18:59 [PATCH] security: Return xattr name from security_dentry_init_security() Vivek Goyal
2021-09-30 20:20 ` Casey Schaufler
@ 2021-10-02 18:10 ` Al Viro
2021-10-04 13:56 ` Vivek Goyal
2021-10-04 15:20 ` Jeff Layton
2 siblings, 1 reply; 11+ messages in thread
From: Al Viro @ 2021-10-02 18:10 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-security-module, selinux, linux-fsdevel, virtio-fs, casey,
Miklos Szeredi, Daniel J Walsh, jlayton, idryomov, ceph-devel,
linux-nfs, bfields, chuck.lever, stephen.smalley.work
On Thu, Sep 30, 2021 at 02:59:10PM -0400, Vivek Goyal wrote:
> Right now security_dentry_init_security() only supports single security
> label and is used by SELinux only. There are two users of of this hook,
> namely ceph and nfs.
>
> NFS does not care about xattr name. Ceph hardcodes the xattr name to
> security.selinux (XATTR_NAME_SELINUX).
>
> I am making changes to fuse/virtiofs to send security label to virtiofsd
> and I need to send xattr name as well. I also hardcoded the name of
> xattr to security.selinux.
>
> Stephen Smalley suggested that it probably is a good idea to modify
> security_dentry_init_security() to also return name of xattr so that
> we can avoid this hardcoding in the callers.
>
> This patch adds a new parameter "const char **xattr_name" to
> security_dentry_init_security() and LSM puts the name of xattr
> too if caller asked for it (xattr_name != NULL).
Umm... Why not return the damn thing on success and ERR_PTR(-E...)
on failure, instead of breeding extra arguments?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-10-02 18:10 ` Al Viro
@ 2021-10-04 13:56 ` Vivek Goyal
0 siblings, 0 replies; 11+ messages in thread
From: Vivek Goyal @ 2021-10-04 13:56 UTC (permalink / raw)
To: Al Viro
Cc: linux-security-module, selinux, linux-fsdevel, virtio-fs, casey,
Miklos Szeredi, Daniel J Walsh, jlayton, idryomov, ceph-devel,
linux-nfs, bfields, chuck.lever, stephen.smalley.work
On Sat, Oct 02, 2021 at 06:10:53PM +0000, Al Viro wrote:
> On Thu, Sep 30, 2021 at 02:59:10PM -0400, Vivek Goyal wrote:
> > Right now security_dentry_init_security() only supports single security
> > label and is used by SELinux only. There are two users of of this hook,
> > namely ceph and nfs.
> >
> > NFS does not care about xattr name. Ceph hardcodes the xattr name to
> > security.selinux (XATTR_NAME_SELINUX).
> >
> > I am making changes to fuse/virtiofs to send security label to virtiofsd
> > and I need to send xattr name as well. I also hardcoded the name of
> > xattr to security.selinux.
> >
> > Stephen Smalley suggested that it probably is a good idea to modify
> > security_dentry_init_security() to also return name of xattr so that
> > we can avoid this hardcoding in the callers.
> >
> > This patch adds a new parameter "const char **xattr_name" to
> > security_dentry_init_security() and LSM puts the name of xattr
> > too if caller asked for it (xattr_name != NULL).
>
> Umm... Why not return the damn thing on success and ERR_PTR(-E...)
> on failure, instead of breeding extra arguments?
Because of the way generic security hook infrastructure is written. There
seem to be only two kind of hooks. Either they return "int" or "void".
And this assumption is built into other helper functions. For example
call_int_hook() and call_void_hook().
So I think it much easier to just add additional parameter and stick
to existing convention of returning an "int", instead of trying to
return a "const char *".
Thanks
Vivek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-09-30 18:59 [PATCH] security: Return xattr name from security_dentry_init_security() Vivek Goyal
2021-09-30 20:20 ` Casey Schaufler
2021-10-02 18:10 ` Al Viro
@ 2021-10-04 15:20 ` Jeff Layton
2021-10-04 15:54 ` Casey Schaufler
2 siblings, 1 reply; 11+ messages in thread
From: Jeff Layton @ 2021-10-04 15:20 UTC (permalink / raw)
To: Vivek Goyal, linux-security-module, selinux
Cc: linux-fsdevel, virtio-fs, casey, Miklos Szeredi, Daniel J Walsh,
idryomov, ceph-devel, linux-nfs, bfields, chuck.lever,
stephen.smalley.work
On Thu, 2021-09-30 at 14:59 -0400, Vivek Goyal wrote:
> Right now security_dentry_init_security() only supports single security
> label and is used by SELinux only. There are two users of of this hook,
> namely ceph and nfs.
>
> NFS does not care about xattr name. Ceph hardcodes the xattr name to
> security.selinux (XATTR_NAME_SELINUX).
>
> I am making changes to fuse/virtiofs to send security label to virtiofsd
> and I need to send xattr name as well. I also hardcoded the name of
> xattr to security.selinux.
>
> Stephen Smalley suggested that it probably is a good idea to modify
> security_dentry_init_security() to also return name of xattr so that
> we can avoid this hardcoding in the callers.
>
> This patch adds a new parameter "const char **xattr_name" to
> security_dentry_init_security() and LSM puts the name of xattr
> too if caller asked for it (xattr_name != NULL).
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>
> I have compile tested this patch. Don't know how to setup ceph and
> test it. Its a very simple change. Hopefully ceph developers can
> have a quick look at it.
>
> A similar attempt was made three years back.
>
> https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
> ---
> fs/ceph/xattr.c | 3 +--
> fs/nfs/nfs4proc.c | 3 ++-
> include/linux/lsm_hook_defs.h | 3 ++-
> include/linux/lsm_hooks.h | 1 +
> include/linux/security.h | 6 ++++--
> security/security.c | 7 ++++---
> security/selinux/hooks.c | 6 +++++-
> 7 files changed, 19 insertions(+), 10 deletions(-)
>
> Index: redhat-linux/security/selinux/hooks.c
> ===================================================================
> --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
> +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
> @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
> }
>
I agree with Al that it would be cleaner to just return the string, but
the call_*_hook stuff makes that a bit more tricky. I suppose this is a
reasonable compromise.
> static int selinux_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> + const struct qstr *name,
> + const char **xattr_name, void **ctx,
> u32 *ctxlen)
> {
> u32 newsid;
> @@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
> if (rc)
> return rc;
>
> + if (xattr_name)
> + *xattr_name = XATTR_NAME_SELINUX;
> +
> return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
> ctxlen);
> }
> Index: redhat-linux/security/security.c
> ===================================================================
> --- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
> +++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
> @@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
> }
>
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen)
> + const struct qstr *name,
> + const char **xattr_name, void **ctx,
> + u32 *ctxlen)
> {
> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> - name, ctx, ctxlen);
> + name, xattr_name, ctx, ctxlen);
> }
> EXPORT_SYMBOL(security_dentry_init_security);
>
> Index: redhat-linux/include/linux/lsm_hooks.h
> ===================================================================
> --- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
> +++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
> @@ -196,6 +196,7 @@
> * @dentry dentry to use in calculating the context.
> * @mode mode used to determine resource type.
> * @name name of the last path component used to create file
> + * @xattr_name pointer to place the pointer to security xattr name
It might be a good idea to also document the lifetime for xattr_name
here. In particular you're returning a pointer to a static string, and
it would be good to note that the caller needn't free it or anything.
> * @ctx pointer to place the pointer to the resulting context in.
> * @ctxlen point to place the length of the resulting context.
> * @dentry_create_files_as:
> Index: redhat-linux/include/linux/security.h
> ===================================================================
> --- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
> +++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
> @@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
> int len, void **mnt_opts);
> int security_move_mount(const struct path *from_path, const struct path *to_path);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen);
> + const struct qstr *name,
> + const char **xattr_name, void **ctx,
> + u32 *ctxlen);
> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> @@ -739,6 +740,7 @@ static inline void security_inode_free(s
> static inline int security_dentry_init_security(struct dentry *dentry,
> int mode,
> const struct qstr *name,
> + const char **xattr_name,
> void **ctx,
> u32 *ctxlen)
> {
> Index: redhat-linux/include/linux/lsm_hook_defs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
> +++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
> @@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
> LSM_HOOK(int, 0, move_mount, const struct path *from_path,
> const struct path *to_path)
> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> - int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
> + int mode, const struct qstr *name, const char **xattr_name,
> + void **ctx, u32 *ctxlen)
> LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> struct qstr *name, const struct cred *old, struct cred *new)
>
> Index: redhat-linux/fs/nfs/nfs4proc.c
> ===================================================================
> --- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
> +++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
> @@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
> 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;
>
> Index: redhat-linux/fs/ceph/xattr.c
> ===================================================================
> --- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
> +++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
> @@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
> int err;
>
> err = security_dentry_init_security(dentry, mode, &dentry->d_name,
> - &as_ctx->sec_ctx,
> + &name, &as_ctx->sec_ctx,
> &as_ctx->sec_ctxlen);
> if (err < 0) {
> WARN_ON_ONCE(err != -EOPNOTSUPP);
> @@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
> * It only supports single security module and only selinux has
> * dentry_init_security hook.
> */
> - name = XATTR_NAME_SELINUX;
> name_len = strlen(name);
> err = ceph_pagelist_reserve(pagelist,
> 4 * 2 + name_len + as_ctx->sec_ctxlen);
>
Looks reasonable overall.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-10-04 15:20 ` Jeff Layton
@ 2021-10-04 15:54 ` Casey Schaufler
2021-10-04 16:01 ` Jeff Layton
0 siblings, 1 reply; 11+ messages in thread
From: Casey Schaufler @ 2021-10-04 15:54 UTC (permalink / raw)
To: Jeff Layton, Vivek Goyal, linux-security-module, selinux
Cc: linux-fsdevel, virtio-fs, Miklos Szeredi, Daniel J Walsh,
idryomov, ceph-devel, linux-nfs, bfields, chuck.lever,
stephen.smalley.work, Casey Schaufler
On 10/4/2021 8:20 AM, Jeff Layton wrote:
> On Thu, 2021-09-30 at 14:59 -0400, Vivek Goyal wrote:
>> Right now security_dentry_init_security() only supports single security
>> label and is used by SELinux only. There are two users of of this hook,
>> namely ceph and nfs.
>>
>> NFS does not care about xattr name. Ceph hardcodes the xattr name to
>> security.selinux (XATTR_NAME_SELINUX).
>>
>> I am making changes to fuse/virtiofs to send security label to virtiofsd
>> and I need to send xattr name as well. I also hardcoded the name of
>> xattr to security.selinux.
>>
>> Stephen Smalley suggested that it probably is a good idea to modify
>> security_dentry_init_security() to also return name of xattr so that
>> we can avoid this hardcoding in the callers.
>>
>> This patch adds a new parameter "const char **xattr_name" to
>> security_dentry_init_security() and LSM puts the name of xattr
>> too if caller asked for it (xattr_name != NULL).
>>
>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> ---
>>
>> I have compile tested this patch. Don't know how to setup ceph and
>> test it. Its a very simple change. Hopefully ceph developers can
>> have a quick look at it.
>>
>> A similar attempt was made three years back.
>>
>> https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
>> ---
>> fs/ceph/xattr.c | 3 +--
>> fs/nfs/nfs4proc.c | 3 ++-
>> include/linux/lsm_hook_defs.h | 3 ++-
>> include/linux/lsm_hooks.h | 1 +
>> include/linux/security.h | 6 ++++--
>> security/security.c | 7 ++++---
>> security/selinux/hooks.c | 6 +++++-
>> 7 files changed, 19 insertions(+), 10 deletions(-)
>>
>> Index: redhat-linux/security/selinux/hooks.c
>> ===================================================================
>> --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
>> +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
>> @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
>> }
>>
> I agree with Al that it would be cleaner to just return the string, but
> the call_*_hook stuff makes that a bit more tricky. I suppose this is a
> reasonable compromise.
call_int_hook() and call_void_hook() were introduced to reduce the monotonous
repetition in the source. They are cosmetic and add no real value. They shouldn't
be a consideration in the discussion.
There is a problem with Al's suggestion. The interface as used today has two real
problems. It returns an attribute value without identifying the attribute. Al's
interface would address this issue. The other problem is that the interface can't
provide multiple attribute+value pairs. The interface is going to need changed to
support that for full module stacking. I don't see a rational way to extend the
interface if it returns a string when there are multiple attributes to choose from.
>> static int selinux_dentry_init_security(struct dentry *dentry, int mode,
>> - const struct qstr *name, void **ctx,
>> + const struct qstr *name,
>> + const char **xattr_name, void **ctx,
>> u32 *ctxlen)
>> {
>> u32 newsid;
>> @@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
>> if (rc)
>> return rc;
>>
>> + if (xattr_name)
>> + *xattr_name = XATTR_NAME_SELINUX;
>> +
>> return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
>> ctxlen);
>> }
>> Index: redhat-linux/security/security.c
>> ===================================================================
>> --- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
>> +++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
>> @@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
>> }
>>
>> int security_dentry_init_security(struct dentry *dentry, int mode,
>> - const struct qstr *name, void **ctx,
>> - u32 *ctxlen)
>> + const struct qstr *name,
>> + const char **xattr_name, void **ctx,
>> + u32 *ctxlen)
>> {
>> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>> - name, ctx, ctxlen);
>> + name, xattr_name, ctx, ctxlen);
>> }
>> EXPORT_SYMBOL(security_dentry_init_security);
>>
>> Index: redhat-linux/include/linux/lsm_hooks.h
>> ===================================================================
>> --- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
>> +++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
>> @@ -196,6 +196,7 @@
>> * @dentry dentry to use in calculating the context.
>> * @mode mode used to determine resource type.
>> * @name name of the last path component used to create file
>> + * @xattr_name pointer to place the pointer to security xattr name
> It might be a good idea to also document the lifetime for xattr_name
> here. In particular you're returning a pointer to a static string, and
> it would be good to note that the caller needn't free it or anything.
>
>> * @ctx pointer to place the pointer to the resulting context in.
>> * @ctxlen point to place the length of the resulting context.
>> * @dentry_create_files_as:
>> Index: redhat-linux/include/linux/security.h
>> ===================================================================
>> --- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
>> +++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
>> @@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
>> int len, void **mnt_opts);
>> int security_move_mount(const struct path *from_path, const struct path *to_path);
>> int security_dentry_init_security(struct dentry *dentry, int mode,
>> - const struct qstr *name, void **ctx,
>> - u32 *ctxlen);
>> + const struct qstr *name,
>> + const char **xattr_name, void **ctx,
>> + u32 *ctxlen);
>> int security_dentry_create_files_as(struct dentry *dentry, int mode,
>> struct qstr *name,
>> const struct cred *old,
>> @@ -739,6 +740,7 @@ static inline void security_inode_free(s
>> static inline int security_dentry_init_security(struct dentry *dentry,
>> int mode,
>> const struct qstr *name,
>> + const char **xattr_name,
>> void **ctx,
>> u32 *ctxlen)
>> {
>> Index: redhat-linux/include/linux/lsm_hook_defs.h
>> ===================================================================
>> --- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
>> +++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
>> @@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
>> LSM_HOOK(int, 0, move_mount, const struct path *from_path,
>> const struct path *to_path)
>> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
>> - int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
>> + int mode, const struct qstr *name, const char **xattr_name,
>> + void **ctx, u32 *ctxlen)
>> LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
>> struct qstr *name, const struct cred *old, struct cred *new)
>>
>> Index: redhat-linux/fs/nfs/nfs4proc.c
>> ===================================================================
>> --- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
>> +++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
>> @@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
>> 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;
>>
>> Index: redhat-linux/fs/ceph/xattr.c
>> ===================================================================
>> --- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
>> +++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
>> @@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
>> int err;
>>
>> err = security_dentry_init_security(dentry, mode, &dentry->d_name,
>> - &as_ctx->sec_ctx,
>> + &name, &as_ctx->sec_ctx,
>> &as_ctx->sec_ctxlen);
>> if (err < 0) {
>> WARN_ON_ONCE(err != -EOPNOTSUPP);
>> @@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
>> * It only supports single security module and only selinux has
>> * dentry_init_security hook.
>> */
>> - name = XATTR_NAME_SELINUX;
>> name_len = strlen(name);
>> err = ceph_pagelist_reserve(pagelist,
>> 4 * 2 + name_len + as_ctx->sec_ctxlen);
>>
> Looks reasonable overall.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-10-04 15:54 ` Casey Schaufler
@ 2021-10-04 16:01 ` Jeff Layton
2021-10-04 16:39 ` Casey Schaufler
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Layton @ 2021-10-04 16:01 UTC (permalink / raw)
To: Casey Schaufler, Vivek Goyal, linux-security-module, selinux
Cc: linux-fsdevel, virtio-fs, Miklos Szeredi, Daniel J Walsh,
idryomov, ceph-devel, linux-nfs, bfields, chuck.lever,
stephen.smalley.work
On Mon, 2021-10-04 at 08:54 -0700, Casey Schaufler wrote:
> On 10/4/2021 8:20 AM, Jeff Layton wrote:
> > On Thu, 2021-09-30 at 14:59 -0400, Vivek Goyal wrote:
> > > Right now security_dentry_init_security() only supports single security
> > > label and is used by SELinux only. There are two users of of this hook,
> > > namely ceph and nfs.
> > >
> > > NFS does not care about xattr name. Ceph hardcodes the xattr name to
> > > security.selinux (XATTR_NAME_SELINUX).
> > >
> > > I am making changes to fuse/virtiofs to send security label to virtiofsd
> > > and I need to send xattr name as well. I also hardcoded the name of
> > > xattr to security.selinux.
> > >
> > > Stephen Smalley suggested that it probably is a good idea to modify
> > > security_dentry_init_security() to also return name of xattr so that
> > > we can avoid this hardcoding in the callers.
> > >
> > > This patch adds a new parameter "const char **xattr_name" to
> > > security_dentry_init_security() and LSM puts the name of xattr
> > > too if caller asked for it (xattr_name != NULL).
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >
> > > I have compile tested this patch. Don't know how to setup ceph and
> > > test it. Its a very simple change. Hopefully ceph developers can
> > > have a quick look at it.
> > >
> > > A similar attempt was made three years back.
> > >
> > > https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
> > > ---
> > > fs/ceph/xattr.c | 3 +--
> > > fs/nfs/nfs4proc.c | 3 ++-
> > > include/linux/lsm_hook_defs.h | 3 ++-
> > > include/linux/lsm_hooks.h | 1 +
> > > include/linux/security.h | 6 ++++--
> > > security/security.c | 7 ++++---
> > > security/selinux/hooks.c | 6 +++++-
> > > 7 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > Index: redhat-linux/security/selinux/hooks.c
> > > ===================================================================
> > > --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
> > > +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
> > > @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
> > > }
> > >
> > I agree with Al that it would be cleaner to just return the string, but
> > the call_*_hook stuff makes that a bit more tricky. I suppose this is a
> > reasonable compromise.
>
> call_int_hook() and call_void_hook() were introduced to reduce the monotonous
> repetition in the source. They are cosmetic and add no real value. They shouldn't
> be a consideration in the discussion.
>
> There is a problem with Al's suggestion. The interface as used today has two real
> problems. It returns an attribute value without identifying the attribute. Al's
> interface would address this issue. The other problem is that the interface can't
> provide multiple attribute+value pairs. The interface is going to need changed to
> support that for full module stacking. I don't see a rational way to extend the
> interface if it returns a string when there are multiple attributes to choose from.
>
Is that also a problem for the ctx parameter? In the case of full module
stacking do you get back multiple contexts as well?
> > > static int selinux_dentry_init_security(struct dentry *dentry, int mode,
> > > - const struct qstr *name, void **ctx,
> > > + const struct qstr *name,
> > > + const char **xattr_name, void **ctx,
> > > u32 *ctxlen)
> > > {
> > > u32 newsid;
> > > @@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
> > > if (rc)
> > > return rc;
> > >
> > > + if (xattr_name)
> > > + *xattr_name = XATTR_NAME_SELINUX;
> > > +
> > > return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
> > > ctxlen);
> > > }
> > > Index: redhat-linux/security/security.c
> > > ===================================================================
> > > --- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
> > > +++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
> > > @@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
> > > }
> > >
> > > int security_dentry_init_security(struct dentry *dentry, int mode,
> > > - const struct qstr *name, void **ctx,
> > > - u32 *ctxlen)
> > > + const struct qstr *name,
> > > + const char **xattr_name, void **ctx,
> > > + u32 *ctxlen)
> > > {
> > > return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> > > - name, ctx, ctxlen);
> > > + name, xattr_name, ctx, ctxlen);
> > > }
> > > EXPORT_SYMBOL(security_dentry_init_security);
> > >
> > > Index: redhat-linux/include/linux/lsm_hooks.h
> > > ===================================================================
> > > --- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
> > > +++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
> > > @@ -196,6 +196,7 @@
> > > * @dentry dentry to use in calculating the context.
> > > * @mode mode used to determine resource type.
> > > * @name name of the last path component used to create file
> > > + * @xattr_name pointer to place the pointer to security xattr name
> > It might be a good idea to also document the lifetime for xattr_name
> > here. In particular you're returning a pointer to a static string, and
> > it would be good to note that the caller needn't free it or anything.
> >
> > > * @ctx pointer to place the pointer to the resulting context in.
> > > * @ctxlen point to place the length of the resulting context.
> > > * @dentry_create_files_as:
> > > Index: redhat-linux/include/linux/security.h
> > > ===================================================================
> > > --- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
> > > +++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
> > > @@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
> > > int len, void **mnt_opts);
> > > int security_move_mount(const struct path *from_path, const struct path *to_path);
> > > int security_dentry_init_security(struct dentry *dentry, int mode,
> > > - const struct qstr *name, void **ctx,
> > > - u32 *ctxlen);
> > > + const struct qstr *name,
> > > + const char **xattr_name, void **ctx,
> > > + u32 *ctxlen);
> > > int security_dentry_create_files_as(struct dentry *dentry, int mode,
> > > struct qstr *name,
> > > const struct cred *old,
> > > @@ -739,6 +740,7 @@ static inline void security_inode_free(s
> > > static inline int security_dentry_init_security(struct dentry *dentry,
> > > int mode,
> > > const struct qstr *name,
> > > + const char **xattr_name,
> > > void **ctx,
> > > u32 *ctxlen)
> > > {
> > > Index: redhat-linux/include/linux/lsm_hook_defs.h
> > > ===================================================================
> > > --- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
> > > +++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
> > > @@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
> > > LSM_HOOK(int, 0, move_mount, const struct path *from_path,
> > > const struct path *to_path)
> > > LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> > > - int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
> > > + int mode, const struct qstr *name, const char **xattr_name,
> > > + void **ctx, u32 *ctxlen)
> > > LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> > > struct qstr *name, const struct cred *old, struct cred *new)
> > >
> > > Index: redhat-linux/fs/nfs/nfs4proc.c
> > > ===================================================================
> > > --- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
> > > +++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
> > > @@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
> > > 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;
> > >
> > > Index: redhat-linux/fs/ceph/xattr.c
> > > ===================================================================
> > > --- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
> > > +++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
> > > @@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
> > > int err;
> > >
> > > err = security_dentry_init_security(dentry, mode, &dentry->d_name,
> > > - &as_ctx->sec_ctx,
> > > + &name, &as_ctx->sec_ctx,
> > > &as_ctx->sec_ctxlen);
> > > if (err < 0) {
> > > WARN_ON_ONCE(err != -EOPNOTSUPP);
> > > @@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
> > > * It only supports single security module and only selinux has
> > > * dentry_init_security hook.
> > > */
> > > - name = XATTR_NAME_SELINUX;
> > > name_len = strlen(name);
> > > err = ceph_pagelist_reserve(pagelist,
> > > 4 * 2 + name_len + as_ctx->sec_ctxlen);
> > >
> > Looks reasonable overall.
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> >
>
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-10-04 16:01 ` Jeff Layton
@ 2021-10-04 16:39 ` Casey Schaufler
2021-10-04 17:05 ` Vivek Goyal
2021-10-04 17:13 ` Vivek Goyal
0 siblings, 2 replies; 11+ messages in thread
From: Casey Schaufler @ 2021-10-04 16:39 UTC (permalink / raw)
To: Jeff Layton, Vivek Goyal, linux-security-module, selinux
Cc: linux-fsdevel, virtio-fs, Miklos Szeredi, Daniel J Walsh,
idryomov, ceph-devel, linux-nfs, bfields, chuck.lever,
stephen.smalley.work, Casey Schaufler
On 10/4/2021 9:01 AM, Jeff Layton wrote:
> On Mon, 2021-10-04 at 08:54 -0700, Casey Schaufler wrote:
>> On 10/4/2021 8:20 AM, Jeff Layton wrote:
>>> On Thu, 2021-09-30 at 14:59 -0400, Vivek Goyal wrote:
>>>> Right now security_dentry_init_security() only supports single security
>>>> label and is used by SELinux only. There are two users of of this hook,
>>>> namely ceph and nfs.
>>>>
>>>> NFS does not care about xattr name. Ceph hardcodes the xattr name to
>>>> security.selinux (XATTR_NAME_SELINUX).
>>>>
>>>> I am making changes to fuse/virtiofs to send security label to virtiofsd
>>>> and I need to send xattr name as well. I also hardcoded the name of
>>>> xattr to security.selinux.
>>>>
>>>> Stephen Smalley suggested that it probably is a good idea to modify
>>>> security_dentry_init_security() to also return name of xattr so that
>>>> we can avoid this hardcoding in the callers.
>>>>
>>>> This patch adds a new parameter "const char **xattr_name" to
>>>> security_dentry_init_security() and LSM puts the name of xattr
>>>> too if caller asked for it (xattr_name != NULL).
>>>>
>>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>>> ---
>>>>
>>>> I have compile tested this patch. Don't know how to setup ceph and
>>>> test it. Its a very simple change. Hopefully ceph developers can
>>>> have a quick look at it.
>>>>
>>>> A similar attempt was made three years back.
>>>>
>>>> https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
>>>> ---
>>>> fs/ceph/xattr.c | 3 +--
>>>> fs/nfs/nfs4proc.c | 3 ++-
>>>> include/linux/lsm_hook_defs.h | 3 ++-
>>>> include/linux/lsm_hooks.h | 1 +
>>>> include/linux/security.h | 6 ++++--
>>>> security/security.c | 7 ++++---
>>>> security/selinux/hooks.c | 6 +++++-
>>>> 7 files changed, 19 insertions(+), 10 deletions(-)
>>>>
>>>> Index: redhat-linux/security/selinux/hooks.c
>>>> ===================================================================
>>>> --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
>>>> +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
>>>> @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
>>>> }
>>>>
>>> I agree with Al that it would be cleaner to just return the string, but
>>> the call_*_hook stuff makes that a bit more tricky. I suppose this is a
>>> reasonable compromise.
>> call_int_hook() and call_void_hook() were introduced to reduce the monotonous
>> repetition in the source. They are cosmetic and add no real value. They shouldn't
>> be a consideration in the discussion.
>>
>> There is a problem with Al's suggestion. The interface as used today has two real
>> problems. It returns an attribute value without identifying the attribute. Al's
>> interface would address this issue. The other problem is that the interface can't
>> provide multiple attribute+value pairs. The interface is going to need changed to
>> support that for full module stacking. I don't see a rational way to extend the
>> interface if it returns a string when there are multiple attributes to choose from.
>>
> Is that also a problem for the ctx parameter? In the case of full module
> stacking do you get back multiple contexts as well?
That's a bigger discussion than is probably appropriate on this thread.
In the module stacking case the caller needs to identify which security
module's context it wants. If the caller is capable of dealing with
multiple attributes (none currently are, but they all assume that you're
using SELinux and only support what SELinux needs) it will need to
do something different. We have chickens and eggs involved. The LSM
infrastructure doesn't need to handle it because none of its callers
are capable of dealing with it. None of the callers try, in part because
they have no way to get the information they would need, and in part
because they don't care about anything beyond SELinux. Ceph, for example,
is hard coded to expect "security.selinux".
On further reflection, Al's suggestion could be made to work if the
caller identified which attribute its looking for.
>>>> static int selinux_dentry_init_security(struct dentry *dentry, int mode,
>>>> - const struct qstr *name, void **ctx,
>>>> + const struct qstr *name,
>>>> + const char **xattr_name, void **ctx,
>>>> u32 *ctxlen)
>>>> {
>>>> u32 newsid;
>>>> @@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
>>>> if (rc)
>>>> return rc;
>>>>
>>>> + if (xattr_name)
>>>> + *xattr_name = XATTR_NAME_SELINUX;
>>>> +
>>>> return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
>>>> ctxlen);
>>>> }
>>>> Index: redhat-linux/security/security.c
>>>> ===================================================================
>>>> --- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
>>>> +++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
>>>> @@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
>>>> }
>>>>
>>>> int security_dentry_init_security(struct dentry *dentry, int mode,
>>>> - const struct qstr *name, void **ctx,
>>>> - u32 *ctxlen)
>>>> + const struct qstr *name,
>>>> + const char **xattr_name, void **ctx,
>>>> + u32 *ctxlen)
>>>> {
>>>> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>>>> - name, ctx, ctxlen);
>>>> + name, xattr_name, ctx, ctxlen);
>>>> }
>>>> EXPORT_SYMBOL(security_dentry_init_security);
>>>>
>>>> Index: redhat-linux/include/linux/lsm_hooks.h
>>>> ===================================================================
>>>> --- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
>>>> +++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
>>>> @@ -196,6 +196,7 @@
>>>> * @dentry dentry to use in calculating the context.
>>>> * @mode mode used to determine resource type.
>>>> * @name name of the last path component used to create file
>>>> + * @xattr_name pointer to place the pointer to security xattr name
>>> It might be a good idea to also document the lifetime for xattr_name
>>> here. In particular you're returning a pointer to a static string, and
>>> it would be good to note that the caller needn't free it or anything.
>>>
>>>> * @ctx pointer to place the pointer to the resulting context in.
>>>> * @ctxlen point to place the length of the resulting context.
>>>> * @dentry_create_files_as:
>>>> Index: redhat-linux/include/linux/security.h
>>>> ===================================================================
>>>> --- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
>>>> +++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
>>>> @@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
>>>> int len, void **mnt_opts);
>>>> int security_move_mount(const struct path *from_path, const struct path *to_path);
>>>> int security_dentry_init_security(struct dentry *dentry, int mode,
>>>> - const struct qstr *name, void **ctx,
>>>> - u32 *ctxlen);
>>>> + const struct qstr *name,
>>>> + const char **xattr_name, void **ctx,
>>>> + u32 *ctxlen);
>>>> int security_dentry_create_files_as(struct dentry *dentry, int mode,
>>>> struct qstr *name,
>>>> const struct cred *old,
>>>> @@ -739,6 +740,7 @@ static inline void security_inode_free(s
>>>> static inline int security_dentry_init_security(struct dentry *dentry,
>>>> int mode,
>>>> const struct qstr *name,
>>>> + const char **xattr_name,
>>>> void **ctx,
>>>> u32 *ctxlen)
>>>> {
>>>> Index: redhat-linux/include/linux/lsm_hook_defs.h
>>>> ===================================================================
>>>> --- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
>>>> +++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
>>>> @@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
>>>> LSM_HOOK(int, 0, move_mount, const struct path *from_path,
>>>> const struct path *to_path)
>>>> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
>>>> - int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
>>>> + int mode, const struct qstr *name, const char **xattr_name,
>>>> + void **ctx, u32 *ctxlen)
>>>> LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
>>>> struct qstr *name, const struct cred *old, struct cred *new)
>>>>
>>>> Index: redhat-linux/fs/nfs/nfs4proc.c
>>>> ===================================================================
>>>> --- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
>>>> +++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
>>>> @@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
>>>> 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;
>>>>
>>>> Index: redhat-linux/fs/ceph/xattr.c
>>>> ===================================================================
>>>> --- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
>>>> +++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
>>>> @@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
>>>> int err;
>>>>
>>>> err = security_dentry_init_security(dentry, mode, &dentry->d_name,
>>>> - &as_ctx->sec_ctx,
>>>> + &name, &as_ctx->sec_ctx,
>>>> &as_ctx->sec_ctxlen);
>>>> if (err < 0) {
>>>> WARN_ON_ONCE(err != -EOPNOTSUPP);
>>>> @@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
>>>> * It only supports single security module and only selinux has
>>>> * dentry_init_security hook.
>>>> */
>>>> - name = XATTR_NAME_SELINUX;
>>>> name_len = strlen(name);
>>>> err = ceph_pagelist_reserve(pagelist,
>>>> 4 * 2 + name_len + as_ctx->sec_ctxlen);
>>>>
>>> Looks reasonable overall.
>>>
>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-10-04 16:39 ` Casey Schaufler
@ 2021-10-04 17:05 ` Vivek Goyal
2021-10-04 17:13 ` Vivek Goyal
1 sibling, 0 replies; 11+ messages in thread
From: Vivek Goyal @ 2021-10-04 17:05 UTC (permalink / raw)
To: Casey Schaufler
Cc: Jeff Layton, linux-security-module, selinux, linux-fsdevel,
virtio-fs, Miklos Szeredi, Daniel J Walsh, idryomov, ceph-devel,
linux-nfs, bfields, chuck.lever, stephen.smalley.work
On Mon, Oct 04, 2021 at 09:39:44AM -0700, Casey Schaufler wrote:
> On 10/4/2021 9:01 AM, Jeff Layton wrote:
> > On Mon, 2021-10-04 at 08:54 -0700, Casey Schaufler wrote:
> >> On 10/4/2021 8:20 AM, Jeff Layton wrote:
> >>> On Thu, 2021-09-30 at 14:59 -0400, Vivek Goyal wrote:
> >>>> Right now security_dentry_init_security() only supports single security
> >>>> label and is used by SELinux only. There are two users of of this hook,
> >>>> namely ceph and nfs.
> >>>>
> >>>> NFS does not care about xattr name. Ceph hardcodes the xattr name to
> >>>> security.selinux (XATTR_NAME_SELINUX).
> >>>>
> >>>> I am making changes to fuse/virtiofs to send security label to virtiofsd
> >>>> and I need to send xattr name as well. I also hardcoded the name of
> >>>> xattr to security.selinux.
> >>>>
> >>>> Stephen Smalley suggested that it probably is a good idea to modify
> >>>> security_dentry_init_security() to also return name of xattr so that
> >>>> we can avoid this hardcoding in the callers.
> >>>>
> >>>> This patch adds a new parameter "const char **xattr_name" to
> >>>> security_dentry_init_security() and LSM puts the name of xattr
> >>>> too if caller asked for it (xattr_name != NULL).
> >>>>
> >>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >>>> ---
> >>>>
> >>>> I have compile tested this patch. Don't know how to setup ceph and
> >>>> test it. Its a very simple change. Hopefully ceph developers can
> >>>> have a quick look at it.
> >>>>
> >>>> A similar attempt was made three years back.
> >>>>
> >>>> https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
> >>>> ---
> >>>> fs/ceph/xattr.c | 3 +--
> >>>> fs/nfs/nfs4proc.c | 3 ++-
> >>>> include/linux/lsm_hook_defs.h | 3 ++-
> >>>> include/linux/lsm_hooks.h | 1 +
> >>>> include/linux/security.h | 6 ++++--
> >>>> security/security.c | 7 ++++---
> >>>> security/selinux/hooks.c | 6 +++++-
> >>>> 7 files changed, 19 insertions(+), 10 deletions(-)
> >>>>
> >>>> Index: redhat-linux/security/selinux/hooks.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
> >>>> +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
> >>>> @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
> >>>> }
> >>>>
> >>> I agree with Al that it would be cleaner to just return the string, but
> >>> the call_*_hook stuff makes that a bit more tricky. I suppose this is a
> >>> reasonable compromise.
> >> call_int_hook() and call_void_hook() were introduced to reduce the monotonous
> >> repetition in the source. They are cosmetic and add no real value. They shouldn't
> >> be a consideration in the discussion.
> >>
> >> There is a problem with Al's suggestion. The interface as used today has two real
> >> problems. It returns an attribute value without identifying the attribute. Al's
> >> interface would address this issue. The other problem is that the interface can't
> >> provide multiple attribute+value pairs. The interface is going to need changed to
> >> support that for full module stacking. I don't see a rational way to extend the
> >> interface if it returns a string when there are multiple attributes to choose from.
> >>
> > Is that also a problem for the ctx parameter? In the case of full module
> > stacking do you get back multiple contexts as well?
>
> That's a bigger discussion than is probably appropriate on this thread.
> In the module stacking case the caller needs to identify which security
> module's context it wants. If the caller is capable of dealing with
> multiple attributes (none currently are, but they all assume that you're
> using SELinux and only support what SELinux needs) it will need to
> do something different. We have chickens and eggs involved. The LSM
> infrastructure doesn't need to handle it because none of its callers
> are capable of dealing with it. None of the callers try, in part because
> they have no way to get the information they would need, and in part
> because they don't care about anything beyond SELinux. Ceph, for example,
> is hard coded to expect "security.selinux".
>
> On further reflection, Al's suggestion could be made to work if the
> caller identified which attribute its looking for.
IMHO, for multiple security contexts we need to write a new hook and
provide capability to deal with lsm stacking. And that new interface
could provide something like taking the name of "xattr" as input and
return corresponding security context (if needed).This existing hook
security_dentry_init_security() can simply be removed once all callers
have migrated to new hook.
So to me real question is, is it worth to work through all generic
LSM stuff and modify it (as needed) so that we can return a
"const char *" instead of "int". I feel complexity of that change
is much more thank simply adding a function parameter. So if were
to compare two options, it feels much simpler to simply add a function
parameter.
Vivek
>
>
> >>>> static int selinux_dentry_init_security(struct dentry *dentry, int mode,
> >>>> - const struct qstr *name, void **ctx,
> >>>> + const struct qstr *name,
> >>>> + const char **xattr_name, void **ctx,
> >>>> u32 *ctxlen)
> >>>> {
> >>>> u32 newsid;
> >>>> @@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
> >>>> if (rc)
> >>>> return rc;
> >>>>
> >>>> + if (xattr_name)
> >>>> + *xattr_name = XATTR_NAME_SELINUX;
> >>>> +
> >>>> return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
> >>>> ctxlen);
> >>>> }
> >>>> Index: redhat-linux/security/security.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
> >>>> +++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
> >>>> @@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
> >>>> }
> >>>>
> >>>> int security_dentry_init_security(struct dentry *dentry, int mode,
> >>>> - const struct qstr *name, void **ctx,
> >>>> - u32 *ctxlen)
> >>>> + const struct qstr *name,
> >>>> + const char **xattr_name, void **ctx,
> >>>> + u32 *ctxlen)
> >>>> {
> >>>> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> >>>> - name, ctx, ctxlen);
> >>>> + name, xattr_name, ctx, ctxlen);
> >>>> }
> >>>> EXPORT_SYMBOL(security_dentry_init_security);
> >>>>
> >>>> Index: redhat-linux/include/linux/lsm_hooks.h
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
> >>>> +++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
> >>>> @@ -196,6 +196,7 @@
> >>>> * @dentry dentry to use in calculating the context.
> >>>> * @mode mode used to determine resource type.
> >>>> * @name name of the last path component used to create file
> >>>> + * @xattr_name pointer to place the pointer to security xattr name
> >>> It might be a good idea to also document the lifetime for xattr_name
> >>> here. In particular you're returning a pointer to a static string, and
> >>> it would be good to note that the caller needn't free it or anything.
> >>>
> >>>> * @ctx pointer to place the pointer to the resulting context in.
> >>>> * @ctxlen point to place the length of the resulting context.
> >>>> * @dentry_create_files_as:
> >>>> Index: redhat-linux/include/linux/security.h
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
> >>>> +++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
> >>>> @@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
> >>>> int len, void **mnt_opts);
> >>>> int security_move_mount(const struct path *from_path, const struct path *to_path);
> >>>> int security_dentry_init_security(struct dentry *dentry, int mode,
> >>>> - const struct qstr *name, void **ctx,
> >>>> - u32 *ctxlen);
> >>>> + const struct qstr *name,
> >>>> + const char **xattr_name, void **ctx,
> >>>> + u32 *ctxlen);
> >>>> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> >>>> struct qstr *name,
> >>>> const struct cred *old,
> >>>> @@ -739,6 +740,7 @@ static inline void security_inode_free(s
> >>>> static inline int security_dentry_init_security(struct dentry *dentry,
> >>>> int mode,
> >>>> const struct qstr *name,
> >>>> + const char **xattr_name,
> >>>> void **ctx,
> >>>> u32 *ctxlen)
> >>>> {
> >>>> Index: redhat-linux/include/linux/lsm_hook_defs.h
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
> >>>> +++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
> >>>> @@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
> >>>> LSM_HOOK(int, 0, move_mount, const struct path *from_path,
> >>>> const struct path *to_path)
> >>>> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> >>>> - int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
> >>>> + int mode, const struct qstr *name, const char **xattr_name,
> >>>> + void **ctx, u32 *ctxlen)
> >>>> LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> >>>> struct qstr *name, const struct cred *old, struct cred *new)
> >>>>
> >>>> Index: redhat-linux/fs/nfs/nfs4proc.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
> >>>> +++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
> >>>> @@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
> >>>> 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;
> >>>>
> >>>> Index: redhat-linux/fs/ceph/xattr.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
> >>>> +++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
> >>>> @@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
> >>>> int err;
> >>>>
> >>>> err = security_dentry_init_security(dentry, mode, &dentry->d_name,
> >>>> - &as_ctx->sec_ctx,
> >>>> + &name, &as_ctx->sec_ctx,
> >>>> &as_ctx->sec_ctxlen);
> >>>> if (err < 0) {
> >>>> WARN_ON_ONCE(err != -EOPNOTSUPP);
> >>>> @@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
> >>>> * It only supports single security module and only selinux has
> >>>> * dentry_init_security hook.
> >>>> */
> >>>> - name = XATTR_NAME_SELINUX;
> >>>> name_len = strlen(name);
> >>>> err = ceph_pagelist_reserve(pagelist,
> >>>> 4 * 2 + name_len + as_ctx->sec_ctxlen);
> >>>>
> >>> Looks reasonable overall.
> >>>
> >>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> >>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-10-04 16:39 ` Casey Schaufler
2021-10-04 17:05 ` Vivek Goyal
@ 2021-10-04 17:13 ` Vivek Goyal
2021-10-04 17:36 ` Casey Schaufler
1 sibling, 1 reply; 11+ messages in thread
From: Vivek Goyal @ 2021-10-04 17:13 UTC (permalink / raw)
To: Casey Schaufler
Cc: Jeff Layton, linux-security-module, selinux, linux-fsdevel,
virtio-fs, Miklos Szeredi, Daniel J Walsh, idryomov, ceph-devel,
linux-nfs, bfields, chuck.lever, stephen.smalley.work
On Mon, Oct 04, 2021 at 09:39:44AM -0700, Casey Schaufler wrote:
> On 10/4/2021 9:01 AM, Jeff Layton wrote:
> > On Mon, 2021-10-04 at 08:54 -0700, Casey Schaufler wrote:
> >> On 10/4/2021 8:20 AM, Jeff Layton wrote:
> >>> On Thu, 2021-09-30 at 14:59 -0400, Vivek Goyal wrote:
> >>>> Right now security_dentry_init_security() only supports single security
> >>>> label and is used by SELinux only. There are two users of of this hook,
> >>>> namely ceph and nfs.
> >>>>
> >>>> NFS does not care about xattr name. Ceph hardcodes the xattr name to
> >>>> security.selinux (XATTR_NAME_SELINUX).
> >>>>
> >>>> I am making changes to fuse/virtiofs to send security label to virtiofsd
> >>>> and I need to send xattr name as well. I also hardcoded the name of
> >>>> xattr to security.selinux.
> >>>>
> >>>> Stephen Smalley suggested that it probably is a good idea to modify
> >>>> security_dentry_init_security() to also return name of xattr so that
> >>>> we can avoid this hardcoding in the callers.
> >>>>
> >>>> This patch adds a new parameter "const char **xattr_name" to
> >>>> security_dentry_init_security() and LSM puts the name of xattr
> >>>> too if caller asked for it (xattr_name != NULL).
> >>>>
> >>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >>>> ---
> >>>>
> >>>> I have compile tested this patch. Don't know how to setup ceph and
> >>>> test it. Its a very simple change. Hopefully ceph developers can
> >>>> have a quick look at it.
> >>>>
> >>>> A similar attempt was made three years back.
> >>>>
> >>>> https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
> >>>> ---
> >>>> fs/ceph/xattr.c | 3 +--
> >>>> fs/nfs/nfs4proc.c | 3 ++-
> >>>> include/linux/lsm_hook_defs.h | 3 ++-
> >>>> include/linux/lsm_hooks.h | 1 +
> >>>> include/linux/security.h | 6 ++++--
> >>>> security/security.c | 7 ++++---
> >>>> security/selinux/hooks.c | 6 +++++-
> >>>> 7 files changed, 19 insertions(+), 10 deletions(-)
> >>>>
> >>>> Index: redhat-linux/security/selinux/hooks.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
> >>>> +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
> >>>> @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
> >>>> }
> >>>>
> >>> I agree with Al that it would be cleaner to just return the string, but
> >>> the call_*_hook stuff makes that a bit more tricky. I suppose this is a
> >>> reasonable compromise.
> >> call_int_hook() and call_void_hook() were introduced to reduce the monotonous
> >> repetition in the source. They are cosmetic and add no real value. They shouldn't
> >> be a consideration in the discussion.
> >>
> >> There is a problem with Al's suggestion. The interface as used today has two real
> >> problems. It returns an attribute value without identifying the attribute. Al's
> >> interface would address this issue. The other problem is that the interface can't
> >> provide multiple attribute+value pairs. The interface is going to need changed to
> >> support that for full module stacking. I don't see a rational way to extend the
> >> interface if it returns a string when there are multiple attributes to choose from.
> >>
> > Is that also a problem for the ctx parameter? In the case of full module
> > stacking do you get back multiple contexts as well?
>
> That's a bigger discussion than is probably appropriate on this thread.
> In the module stacking case the caller needs to identify which security
> module's context it wants. If the caller is capable of dealing with
> multiple attributes (none currently are, but they all assume that you're
> using SELinux and only support what SELinux needs) it will need to
> do something different. We have chickens and eggs involved. The LSM
> infrastructure doesn't need to handle it because none of its callers
> are capable of dealing with it. None of the callers try, in part because
> they have no way to get the information they would need, and in part
> because they don't care about anything beyond SELinux. Ceph, for example,
> is hard coded to expect "security.selinux".
>
> On further reflection, Al's suggestion could be made to work if the
> caller identified which attribute its looking for.
Or I could just add a parameter "const char *xattr_name" which identifies
which xattr caller is looking for. (So no returning the name of xattr).
And this will simply return "int".
Anyway, all the callers right now only expect "security.selinux". Those
who can deal with other xattrs, can pass it explicitly.
I feel current patch is better because caller can check anyway, what
xattr name it got in return and reject it if it does not want to deal
with it. No hardcoding of xattr names required.
Vivek
>
>
> >>>> static int selinux_dentry_init_security(struct dentry *dentry, int mode,
> >>>> - const struct qstr *name, void **ctx,
> >>>> + const struct qstr *name,
> >>>> + const char **xattr_name, void **ctx,
> >>>> u32 *ctxlen)
> >>>> {
> >>>> u32 newsid;
> >>>> @@ -2961,6 +2962,9 @@ static int selinux_dentry_init_security(
> >>>> if (rc)
> >>>> return rc;
> >>>>
> >>>> + if (xattr_name)
> >>>> + *xattr_name = XATTR_NAME_SELINUX;
> >>>> +
> >>>> return security_sid_to_context(&selinux_state, newsid, (char **)ctx,
> >>>> ctxlen);
> >>>> }
> >>>> Index: redhat-linux/security/security.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/security/security.c 2021-08-16 10:39:28.518988836 -0400
> >>>> +++ redhat-linux/security/security.c 2021-09-30 13:54:36.367195347 -0400
> >>>> @@ -1052,11 +1052,12 @@ void security_inode_free(struct inode *i
> >>>> }
> >>>>
> >>>> int security_dentry_init_security(struct dentry *dentry, int mode,
> >>>> - const struct qstr *name, void **ctx,
> >>>> - u32 *ctxlen)
> >>>> + const struct qstr *name,
> >>>> + const char **xattr_name, void **ctx,
> >>>> + u32 *ctxlen)
> >>>> {
> >>>> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> >>>> - name, ctx, ctxlen);
> >>>> + name, xattr_name, ctx, ctxlen);
> >>>> }
> >>>> EXPORT_SYMBOL(security_dentry_init_security);
> >>>>
> >>>> Index: redhat-linux/include/linux/lsm_hooks.h
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/include/linux/lsm_hooks.h 2021-06-02 10:20:27.717485143 -0400
> >>>> +++ redhat-linux/include/linux/lsm_hooks.h 2021-09-30 13:56:48.440195347 -0400
> >>>> @@ -196,6 +196,7 @@
> >>>> * @dentry dentry to use in calculating the context.
> >>>> * @mode mode used to determine resource type.
> >>>> * @name name of the last path component used to create file
> >>>> + * @xattr_name pointer to place the pointer to security xattr name
> >>> It might be a good idea to also document the lifetime for xattr_name
> >>> here. In particular you're returning a pointer to a static string, and
> >>> it would be good to note that the caller needn't free it or anything.
> >>>
> >>>> * @ctx pointer to place the pointer to the resulting context in.
> >>>> * @ctxlen point to place the length of the resulting context.
> >>>> * @dentry_create_files_as:
> >>>> Index: redhat-linux/include/linux/security.h
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/include/linux/security.h 2021-08-16 10:39:28.484988836 -0400
> >>>> +++ redhat-linux/include/linux/security.h 2021-09-30 13:59:00.288195347 -0400
> >>>> @@ -317,8 +317,9 @@ int security_add_mnt_opt(const char *opt
> >>>> int len, void **mnt_opts);
> >>>> int security_move_mount(const struct path *from_path, const struct path *to_path);
> >>>> int security_dentry_init_security(struct dentry *dentry, int mode,
> >>>> - const struct qstr *name, void **ctx,
> >>>> - u32 *ctxlen);
> >>>> + const struct qstr *name,
> >>>> + const char **xattr_name, void **ctx,
> >>>> + u32 *ctxlen);
> >>>> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> >>>> struct qstr *name,
> >>>> const struct cred *old,
> >>>> @@ -739,6 +740,7 @@ static inline void security_inode_free(s
> >>>> static inline int security_dentry_init_security(struct dentry *dentry,
> >>>> int mode,
> >>>> const struct qstr *name,
> >>>> + const char **xattr_name,
> >>>> void **ctx,
> >>>> u32 *ctxlen)
> >>>> {
> >>>> Index: redhat-linux/include/linux/lsm_hook_defs.h
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/include/linux/lsm_hook_defs.h 2021-07-07 11:54:59.673549151 -0400
> >>>> +++ redhat-linux/include/linux/lsm_hook_defs.h 2021-09-30 14:02:13.114195347 -0400
> >>>> @@ -83,7 +83,8 @@ LSM_HOOK(int, 0, sb_add_mnt_opt, const c
> >>>> LSM_HOOK(int, 0, move_mount, const struct path *from_path,
> >>>> const struct path *to_path)
> >>>> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> >>>> - int mode, const struct qstr *name, void **ctx, u32 *ctxlen)
> >>>> + int mode, const struct qstr *name, const char **xattr_name,
> >>>> + void **ctx, u32 *ctxlen)
> >>>> LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> >>>> struct qstr *name, const struct cred *old, struct cred *new)
> >>>>
> >>>> Index: redhat-linux/fs/nfs/nfs4proc.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/fs/nfs/nfs4proc.c 2021-07-14 14:47:42.732842926 -0400
> >>>> +++ redhat-linux/fs/nfs/nfs4proc.c 2021-09-30 14:06:02.249195347 -0400
> >>>> @@ -127,7 +127,8 @@ nfs4_label_init_security(struct inode *d
> >>>> 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;
> >>>>
> >>>> Index: redhat-linux/fs/ceph/xattr.c
> >>>> ===================================================================
> >>>> --- redhat-linux.orig/fs/ceph/xattr.c 2021-09-09 13:05:21.800611264 -0400
> >>>> +++ redhat-linux/fs/ceph/xattr.c 2021-09-30 14:14:59.892195347 -0400
> >>>> @@ -1311,7 +1311,7 @@ int ceph_security_init_secctx(struct den
> >>>> int err;
> >>>>
> >>>> err = security_dentry_init_security(dentry, mode, &dentry->d_name,
> >>>> - &as_ctx->sec_ctx,
> >>>> + &name, &as_ctx->sec_ctx,
> >>>> &as_ctx->sec_ctxlen);
> >>>> if (err < 0) {
> >>>> WARN_ON_ONCE(err != -EOPNOTSUPP);
> >>>> @@ -1335,7 +1335,6 @@ int ceph_security_init_secctx(struct den
> >>>> * It only supports single security module and only selinux has
> >>>> * dentry_init_security hook.
> >>>> */
> >>>> - name = XATTR_NAME_SELINUX;
> >>>> name_len = strlen(name);
> >>>> err = ceph_pagelist_reserve(pagelist,
> >>>> 4 * 2 + name_len + as_ctx->sec_ctxlen);
> >>>>
> >>> Looks reasonable overall.
> >>>
> >>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> >>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] security: Return xattr name from security_dentry_init_security()
2021-10-04 17:13 ` Vivek Goyal
@ 2021-10-04 17:36 ` Casey Schaufler
0 siblings, 0 replies; 11+ messages in thread
From: Casey Schaufler @ 2021-10-04 17:36 UTC (permalink / raw)
To: Vivek Goyal
Cc: Jeff Layton, linux-security-module, selinux, linux-fsdevel,
virtio-fs, Miklos Szeredi, Daniel J Walsh, idryomov, ceph-devel,
linux-nfs, bfields, chuck.lever, stephen.smalley.work,
Casey Schaufler
On 10/4/2021 10:13 AM, Vivek Goyal wrote:
> On Mon, Oct 04, 2021 at 09:39:44AM -0700, Casey Schaufler wrote:
>> On 10/4/2021 9:01 AM, Jeff Layton wrote:
>>> On Mon, 2021-10-04 at 08:54 -0700, Casey Schaufler wrote:
>>>> On 10/4/2021 8:20 AM, Jeff Layton wrote:
>>>>> On Thu, 2021-09-30 at 14:59 -0400, Vivek Goyal wrote:
>>>>>> Right now security_dentry_init_security() only supports single security
>>>>>> label and is used by SELinux only. There are two users of of this hook,
>>>>>> namely ceph and nfs.
>>>>>>
>>>>>> NFS does not care about xattr name. Ceph hardcodes the xattr name to
>>>>>> security.selinux (XATTR_NAME_SELINUX).
>>>>>>
>>>>>> I am making changes to fuse/virtiofs to send security label to virtiofsd
>>>>>> and I need to send xattr name as well. I also hardcoded the name of
>>>>>> xattr to security.selinux.
>>>>>>
>>>>>> Stephen Smalley suggested that it probably is a good idea to modify
>>>>>> security_dentry_init_security() to also return name of xattr so that
>>>>>> we can avoid this hardcoding in the callers.
>>>>>>
>>>>>> This patch adds a new parameter "const char **xattr_name" to
>>>>>> security_dentry_init_security() and LSM puts the name of xattr
>>>>>> too if caller asked for it (xattr_name != NULL).
>>>>>>
>>>>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>>>>> ---
>>>>>>
>>>>>> I have compile tested this patch. Don't know how to setup ceph and
>>>>>> test it. Its a very simple change. Hopefully ceph developers can
>>>>>> have a quick look at it.
>>>>>>
>>>>>> A similar attempt was made three years back.
>>>>>>
>>>>>> https://lore.kernel.org/linux-security-module/20180626080429.27304-1-zyan@redhat.com/T/
>>>>>> ---
>>>>>> fs/ceph/xattr.c | 3 +--
>>>>>> fs/nfs/nfs4proc.c | 3 ++-
>>>>>> include/linux/lsm_hook_defs.h | 3 ++-
>>>>>> include/linux/lsm_hooks.h | 1 +
>>>>>> include/linux/security.h | 6 ++++--
>>>>>> security/security.c | 7 ++++---
>>>>>> security/selinux/hooks.c | 6 +++++-
>>>>>> 7 files changed, 19 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> Index: redhat-linux/security/selinux/hooks.c
>>>>>> ===================================================================
>>>>>> --- redhat-linux.orig/security/selinux/hooks.c 2021-09-28 11:36:03.559785943 -0400
>>>>>> +++ redhat-linux/security/selinux/hooks.c 2021-09-30 14:01:05.869195347 -0400
>>>>>> @@ -2948,7 +2948,8 @@ static void selinux_inode_free_security(
>>>>>> }
>>>>>>
>>>>> I agree with Al that it would be cleaner to just return the string, but
>>>>> the call_*_hook stuff makes that a bit more tricky. I suppose this is a
>>>>> reasonable compromise.
>>>> call_int_hook() and call_void_hook() were introduced to reduce the monotonous
>>>> repetition in the source. They are cosmetic and add no real value. They shouldn't
>>>> be a consideration in the discussion.
>>>>
>>>> There is a problem with Al's suggestion. The interface as used today has two real
>>>> problems. It returns an attribute value without identifying the attribute. Al's
>>>> interface would address this issue. The other problem is that the interface can't
>>>> provide multiple attribute+value pairs. The interface is going to need changed to
>>>> support that for full module stacking. I don't see a rational way to extend the
>>>> interface if it returns a string when there are multiple attributes to choose from.
>>>>
>>> Is that also a problem for the ctx parameter? In the case of full module
>>> stacking do you get back multiple contexts as well?
>> That's a bigger discussion than is probably appropriate on this thread.
>> In the module stacking case the caller needs to identify which security
>> module's context it wants. If the caller is capable of dealing with
>> multiple attributes (none currently are, but they all assume that you're
>> using SELinux and only support what SELinux needs) it will need to
>> do something different. We have chickens and eggs involved. The LSM
>> infrastructure doesn't need to handle it because none of its callers
>> are capable of dealing with it. None of the callers try, in part because
>> they have no way to get the information they would need, and in part
>> because they don't care about anything beyond SELinux. Ceph, for example,
>> is hard coded to expect "security.selinux".
>>
>> On further reflection, Al's suggestion could be made to work if the
>> caller identified which attribute its looking for.
> Or I could just add a parameter "const char *xattr_name" which identifies
> which xattr caller is looking for. (So no returning the name of xattr).
> And this will simply return "int".
>
> Anyway, all the callers right now only expect "security.selinux". Those
> who can deal with other xattrs, can pass it explicitly.
Labeled NFS was supposed to support Smack as well as SELinux.
To the best of my knowledge no one ever actually got that working.
But is was promised, and there's no obvious reason it shouldn't work.
> I feel current patch is better because caller can check anyway, what
> xattr name it got in return and reject it if it does not want to deal
> with it. No hardcoding of xattr names required.
There are a bunch of LSM hooks that I'd love to rip out and replace
with more rational interfaces. This is one. I doubt I'll have the
opportunity to do that in this lifetime. Any change that works is
OK with me. There's probably some amount of change that will be
necessary in the future, and neither of the proposals on the table
are unworkable.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-10-04 17:36 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30 18:59 [PATCH] security: Return xattr name from security_dentry_init_security() Vivek Goyal
2021-09-30 20:20 ` Casey Schaufler
2021-10-02 18:10 ` Al Viro
2021-10-04 13:56 ` Vivek Goyal
2021-10-04 15:20 ` Jeff Layton
2021-10-04 15:54 ` Casey Schaufler
2021-10-04 16:01 ` Jeff Layton
2021-10-04 16:39 ` Casey Schaufler
2021-10-04 17:05 ` Vivek Goyal
2021-10-04 17:13 ` Vivek Goyal
2021-10-04 17:36 ` Casey Schaufler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).