All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality
@ 2020-11-05 17:33 Olga Kornievskaia
  2020-11-05 17:33 ` [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state Olga Kornievskaia
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Olga Kornievskaia @ 2020-11-05 17:33 UTC (permalink / raw)
  To: trond.myklebust, anna.schumaker, paul, stephen.smalley.work
  Cc: linux-nfs, linux-security-module, selinux

From: Olga Kornievskaia <kolga@netapp.com>

Add a new hook func_query_vfs to query an LSM module (such as
SELinux) with the intention of finding whether or not it is enabled
or perhaps supports some functionality.

NFS stores security labels for file system objects and SElinux
or any other LSM module can query those objects. But it's
wasteful to do so when no security enforcement is taking place.
Using a new API call of security_func_query_vfs() and asking if

Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/lsm_hook_defs.h | 1 +
 include/linux/security.h      | 4 ++++
 security/security.c           | 6 ++++++
 security/selinux/hooks.c      | 7 +++++++
 4 files changed, 18 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 32a940117e7a..df3454a1fcac 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -257,6 +257,7 @@ LSM_HOOK(int, 0, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
 LSM_HOOK(int, 0, inode_setsecctx, struct dentry *dentry, void *ctx, u32 ctxlen)
 LSM_HOOK(int, 0, inode_getsecctx, struct inode *inode, void **ctx,
 	 u32 *ctxlen)
+LSM_HOOK(int, 0, func_query_vfs, unsigned int)
 
 #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
 LSM_HOOK(int, 0, post_notification, const struct cred *w_cred,
diff --git a/include/linux/security.h b/include/linux/security.h
index bc2725491560..e15964059fa4 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -456,6 +456,10 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
 int security_locked_down(enum lockdown_reason what);
+#define LSM_FQUERY_VFS_NONE     0x00000000
+#define LSM_FQUERY_VFS_XATTRS   0x00000001
+int security_func_query_vfs(unsigned int flags);
+
 #else /* CONFIG_SECURITY */
 
 static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
diff --git a/security/security.c b/security/security.c
index a28045dc9e7f..502b33865238 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2067,6 +2067,12 @@ int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 }
 EXPORT_SYMBOL(security_inode_getsecctx);
 
+int security_func_query_vfs(unsigned int flags)
+{
+	return call_int_hook(func_query_vfs, 0, flags);
+}
+EXPORT_SYMBOL(security_func_query_vfs);
+
 #ifdef CONFIG_WATCH_QUEUE
 int security_post_notification(const struct cred *w_cred,
 			       const struct cred *cred,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6b1826fc3658..38f47570e214 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,7 @@
 #include <uapi/linux/mount.h>
 #include <linux/fsnotify.h>
 #include <linux/fanotify.h>
+#include <linux/security.h>
 
 #include "avc.h"
 #include "objsec.h"
@@ -6502,6 +6503,11 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
 	spin_unlock(&isec->lock);
 }
 
+static int selinux_func_query_vfs(unsigned int flags)
+{
+	return !!(flags & LSM_FQUERY_VFS_XATTRS);
+}
+
 /*
  *	called with inode->i_mutex locked
  */
@@ -7085,6 +7091,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(inode_invalidate_secctx, selinux_inode_invalidate_secctx),
 	LSM_HOOK_INIT(inode_notifysecctx, selinux_inode_notifysecctx),
 	LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
+	LSM_HOOK_INIT(func_query_vfs, selinux_func_query_vfs),
 
 	LSM_HOOK_INIT(unix_stream_connect, selinux_socket_unix_stream_connect),
 	LSM_HOOK_INIT(unix_may_send, selinux_socket_unix_may_send),
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 17:33 [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Olga Kornievskaia
@ 2020-11-05 17:33 ` Olga Kornievskaia
  2020-11-05 18:55   ` Ondrej Mosnacek
  2020-11-05 23:06     ` kernel test robot
  2020-11-05 19:39 ` [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Casey Schaufler
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Olga Kornievskaia @ 2020-11-05 17:33 UTC (permalink / raw)
  To: trond.myklebust, anna.schumaker, paul, stephen.smalley.work
  Cc: linux-nfs, linux-security-module, selinux

From: Olga Kornievskaia <kolga@netapp.com>

Currently, the client will always ask for security_labels if the server
returns that it supports that feature regardless of any LSM modules
(such as Selinux) enforcing security policy. This adds performance
penalty to the READDIR operation.

Instead, query the LSM module to find if anything is enabled and
if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.

Suggested-by: Scott Mayhew <smayhew@redhat.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c       | 5 +++++
 fs/nfs/nfs4xdr.c        | 3 ++-
 include/linux/nfs_xdr.h | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e0ca9b2b210..774bc5e63ca7 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -55,6 +55,7 @@
 #include <linux/utsname.h>
 #include <linux/freezer.h>
 #include <linux/iversion.h>
+#include <linux/security.h>
 
 #include "nfs4_fs.h"
 #include "delegation.h"
@@ -4968,6 +4969,7 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
 		.count = count,
 		.bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
 		.plus = plus,
+		.labels = true,
 	};
 	struct nfs4_readdir_res res;
 	struct rpc_message msg = {
@@ -4977,10 +4979,13 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
 		.rpc_cred = cred,
 	};
 	int			status;
+	int sec_flags = LSM_FQUERY_VFS_XATTRS;
 
 	dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
 			dentry,
 			(unsigned long long)cookie);
+	if (!security_func_query_vfs(sec_flags))
+		args.labels = false;
 	nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
 	res.pgbase = args.pgbase;
 	status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c6dbfcae7517..585d5b5cc3dc 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1605,7 +1605,8 @@ static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg
 			FATTR4_WORD1_OWNER_GROUP|FATTR4_WORD1_RAWDEV|
 			FATTR4_WORD1_SPACE_USED|FATTR4_WORD1_TIME_ACCESS|
 			FATTR4_WORD1_TIME_METADATA|FATTR4_WORD1_TIME_MODIFY;
-		attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
+		if (readdir->labels)
+			attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
 		dircount >>= 1;
 	}
 	/* Use mounted_on_fileid only if the server supports it */
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index d63cb862d58e..95f648b26525 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1119,6 +1119,7 @@ struct nfs4_readdir_arg {
 	unsigned int			pgbase;	/* zero-copy data */
 	const u32 *			bitmask;
 	bool				plus;
+	bool				labels;
 };
 
 struct nfs4_readdir_res {
-- 
2.18.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 17:33 ` [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state Olga Kornievskaia
@ 2020-11-05 18:55   ` Ondrej Mosnacek
  2020-11-05 19:51     ` Olga Kornievskaia
  2020-11-05 23:06     ` kernel test robot
  1 sibling, 1 reply; 13+ messages in thread
From: Ondrej Mosnacek @ 2020-11-05 18:55 UTC (permalink / raw)
  To: Olga Kornievskaia
  Cc: trond.myklebust, anna.schumaker, Paul Moore, Stephen Smalley,
	linux-nfs, Linux Security Module list, SElinux list

On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
>
> Currently, the client will always ask for security_labels if the server
> returns that it supports that feature regardless of any LSM modules
> (such as Selinux) enforcing security policy. This adds performance
> penalty to the READDIR operation.
>
> Instead, query the LSM module to find if anything is enabled and
> if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.

Having spent some time staring at some of the NFS code very recently,
I can't help but suggest: Would it perhaps be enough to decide whether
to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some LSM
confirms via the security_sb_*_mnt_opts() hook that it wants the
filesystem to give it labels (or at least that's how I interpret the
cryptic name) [1]. It's just a shot in the dark, but it seems to fit
this use case.

[1] https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148

>
> Suggested-by: Scott Mayhew <smayhew@redhat.com>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c       | 5 +++++
>  fs/nfs/nfs4xdr.c        | 3 ++-
>  include/linux/nfs_xdr.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9e0ca9b2b210..774bc5e63ca7 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -55,6 +55,7 @@
>  #include <linux/utsname.h>
>  #include <linux/freezer.h>
>  #include <linux/iversion.h>
> +#include <linux/security.h>
>
>  #include "nfs4_fs.h"
>  #include "delegation.h"
> @@ -4968,6 +4969,7 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
>                 .count = count,
>                 .bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
>                 .plus = plus,
> +               .labels = true,
>         };
>         struct nfs4_readdir_res res;
>         struct rpc_message msg = {
> @@ -4977,10 +4979,13 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
>                 .rpc_cred = cred,
>         };
>         int                     status;
> +       int sec_flags = LSM_FQUERY_VFS_XATTRS;
>
>         dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
>                         dentry,
>                         (unsigned long long)cookie);
> +       if (!security_func_query_vfs(sec_flags))
> +               args.labels = false;
>         nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
>         res.pgbase = args.pgbase;
>         status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index c6dbfcae7517..585d5b5cc3dc 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1605,7 +1605,8 @@ static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg
>                         FATTR4_WORD1_OWNER_GROUP|FATTR4_WORD1_RAWDEV|
>                         FATTR4_WORD1_SPACE_USED|FATTR4_WORD1_TIME_ACCESS|
>                         FATTR4_WORD1_TIME_METADATA|FATTR4_WORD1_TIME_MODIFY;
> -               attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
> +               if (readdir->labels)
> +                       attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
>                 dircount >>= 1;
>         }
>         /* Use mounted_on_fileid only if the server supports it */
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index d63cb862d58e..95f648b26525 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1119,6 +1119,7 @@ struct nfs4_readdir_arg {
>         unsigned int                    pgbase; /* zero-copy data */
>         const u32 *                     bitmask;
>         bool                            plus;
> +       bool                            labels;
>  };
>
>  struct nfs4_readdir_res {
> --
> 2.18.2
>


-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality
  2020-11-05 17:33 [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Olga Kornievskaia
  2020-11-05 17:33 ` [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state Olga Kornievskaia
@ 2020-11-05 19:39 ` Casey Schaufler
  2020-11-07  1:33 ` James Morris
  2020-11-14 10:12 ` kernel test robot
  3 siblings, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2020-11-05 19:39 UTC (permalink / raw)
  To: Olga Kornievskaia, trond.myklebust, anna.schumaker, paul,
	stephen.smalley.work
  Cc: linux-nfs, linux-security-module, selinux, Casey Schaufler

On 11/5/2020 9:33 AM, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
>
> Add a new hook func_query_vfs to query an LSM module (such as
> SELinux) with the intention of finding whether or not it is enabled
> or perhaps supports some functionality.
>
> NFS stores security labels for file system objects and SElinux
> or any other LSM module can query those objects. But it's
> wasteful to do so when no security enforcement is taking place.
> Using a new API call of security_func_query_vfs() and asking if

As I mentioned earlier, this is a sub-optimal implementation.
Instead of adding a func_query_vfs() hook add a field to
security_hook_list and have the LSM declare the "features"
it supports there. security_add_hooks() can gather the "features"
for the whole system into a single mask. security_func_query_vfs()
can then just return that mask.

> Suggested-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  include/linux/lsm_hook_defs.h | 1 +
>  include/linux/security.h      | 4 ++++
>  security/security.c           | 6 ++++++
>  security/selinux/hooks.c      | 7 +++++++
>  4 files changed, 18 insertions(+)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 32a940117e7a..df3454a1fcac 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -257,6 +257,7 @@ LSM_HOOK(int, 0, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
>  LSM_HOOK(int, 0, inode_setsecctx, struct dentry *dentry, void *ctx, u32 ctxlen)
>  LSM_HOOK(int, 0, inode_getsecctx, struct inode *inode, void **ctx,
>  	 u32 *ctxlen)
> +LSM_HOOK(int, 0, func_query_vfs, unsigned int)
>  
>  #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
>  LSM_HOOK(int, 0, post_notification, const struct cred *w_cred,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index bc2725491560..e15964059fa4 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -456,6 +456,10 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
>  int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
>  int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
>  int security_locked_down(enum lockdown_reason what);
> +#define LSM_FQUERY_VFS_NONE     0x00000000
> +#define LSM_FQUERY_VFS_XATTRS   0x00000001

This is the wrong granularity for the task at hand.
You don't care if an LSM supports vfs xattrs. You care if
the LSM provides an xattr that may be propagated by NFS
for the purpose of mandatory access control. An LSM that
provides time-of-day controls may use xattrs that are
meaningless to NFS.


> +int security_func_query_vfs(unsigned int flags);
> +
>  #else /* CONFIG_SECURITY */
>  
>  static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> diff --git a/security/security.c b/security/security.c
> index a28045dc9e7f..502b33865238 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2067,6 +2067,12 @@ int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  }
>  EXPORT_SYMBOL(security_inode_getsecctx);
>  
> +int security_func_query_vfs(unsigned int flags)
> +{
> +	return call_int_hook(func_query_vfs, 0, flags);
> +}
> +EXPORT_SYMBOL(security_func_query_vfs);
> +
>  #ifdef CONFIG_WATCH_QUEUE
>  int security_post_notification(const struct cred *w_cred,
>  			       const struct cred *cred,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6b1826fc3658..38f47570e214 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -92,6 +92,7 @@
>  #include <uapi/linux/mount.h>
>  #include <linux/fsnotify.h>
>  #include <linux/fanotify.h>
> +#include <linux/security.h>
>  
>  #include "avc.h"
>  #include "objsec.h"
> @@ -6502,6 +6503,11 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
>  	spin_unlock(&isec->lock);
>  }
>  
> +static int selinux_func_query_vfs(unsigned int flags)
> +{
> +	return !!(flags & LSM_FQUERY_VFS_XATTRS);
> +}
> +
>  /*
>   *	called with inode->i_mutex locked
>   */
> @@ -7085,6 +7091,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(inode_invalidate_secctx, selinux_inode_invalidate_secctx),
>  	LSM_HOOK_INIT(inode_notifysecctx, selinux_inode_notifysecctx),
>  	LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
> +	LSM_HOOK_INIT(func_query_vfs, selinux_func_query_vfs),
>  
>  	LSM_HOOK_INIT(unix_stream_connect, selinux_socket_unix_stream_connect),
>  	LSM_HOOK_INIT(unix_may_send, selinux_socket_unix_may_send),


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 18:55   ` Ondrej Mosnacek
@ 2020-11-05 19:51     ` Olga Kornievskaia
  2020-11-05 20:24       ` Ondrej Mosnacek
  2020-11-05 21:18       ` Trond Myklebust
  0 siblings, 2 replies; 13+ messages in thread
From: Olga Kornievskaia @ 2020-11-05 19:51 UTC (permalink / raw)
  To: Ondrej Mosnacek
  Cc: Trond Myklebust, Anna Schumaker, Paul Moore, Stephen Smalley,
	linux-nfs, Linux Security Module list, SElinux list

On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> <olga.kornievskaia@gmail.com> wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Currently, the client will always ask for security_labels if the server
> > returns that it supports that feature regardless of any LSM modules
> > (such as Selinux) enforcing security policy. This adds performance
> > penalty to the READDIR operation.
> >
> > Instead, query the LSM module to find if anything is enabled and
> > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
>
> Having spent some time staring at some of the NFS code very recently,
> I can't help but suggest: Would it perhaps be enough to decide whether
> to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some LSM
> confirms via the security_sb_*_mnt_opts() hook that it wants the
> filesystem to give it labels (or at least that's how I interpret the
> cryptic name) [1]. It's just a shot in the dark, but it seems to fit
> this use case.
>
> [1] https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148

Very interesting. I was not aware of something like that nor was it
mentioned when I asked on the selinux mailing list. I wonder if this
is a supported feature that will always stay? In that case, NFS
wouldn't need the extra hook that was added for this series. I will
try this out and report back.

>
> >
> > Suggested-by: Scott Mayhew <smayhew@redhat.com>
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs4proc.c       | 5 +++++
> >  fs/nfs/nfs4xdr.c        | 3 ++-
> >  include/linux/nfs_xdr.h | 1 +
> >  3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 9e0ca9b2b210..774bc5e63ca7 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -55,6 +55,7 @@
> >  #include <linux/utsname.h>
> >  #include <linux/freezer.h>
> >  #include <linux/iversion.h>
> > +#include <linux/security.h>
> >
> >  #include "nfs4_fs.h"
> >  #include "delegation.h"
> > @@ -4968,6 +4969,7 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
> >                 .count = count,
> >                 .bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
> >                 .plus = plus,
> > +               .labels = true,
> >         };
> >         struct nfs4_readdir_res res;
> >         struct rpc_message msg = {
> > @@ -4977,10 +4979,13 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
> >                 .rpc_cred = cred,
> >         };
> >         int                     status;
> > +       int sec_flags = LSM_FQUERY_VFS_XATTRS;
> >
> >         dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
> >                         dentry,
> >                         (unsigned long long)cookie);
> > +       if (!security_func_query_vfs(sec_flags))
> > +               args.labels = false;
> >         nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
> >         res.pgbase = args.pgbase;
> >         status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
> > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > index c6dbfcae7517..585d5b5cc3dc 100644
> > --- a/fs/nfs/nfs4xdr.c
> > +++ b/fs/nfs/nfs4xdr.c
> > @@ -1605,7 +1605,8 @@ static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg
> >                         FATTR4_WORD1_OWNER_GROUP|FATTR4_WORD1_RAWDEV|
> >                         FATTR4_WORD1_SPACE_USED|FATTR4_WORD1_TIME_ACCESS|
> >                         FATTR4_WORD1_TIME_METADATA|FATTR4_WORD1_TIME_MODIFY;
> > -               attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
> > +               if (readdir->labels)
> > +                       attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
> >                 dircount >>= 1;
> >         }
> >         /* Use mounted_on_fileid only if the server supports it */
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index d63cb862d58e..95f648b26525 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -1119,6 +1119,7 @@ struct nfs4_readdir_arg {
> >         unsigned int                    pgbase; /* zero-copy data */
> >         const u32 *                     bitmask;
> >         bool                            plus;
> > +       bool                            labels;
> >  };
> >
> >  struct nfs4_readdir_res {
> > --
> > 2.18.2
> >
>
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 19:51     ` Olga Kornievskaia
@ 2020-11-05 20:24       ` Ondrej Mosnacek
  2020-11-05 21:18       ` Trond Myklebust
  1 sibling, 0 replies; 13+ messages in thread
From: Ondrej Mosnacek @ 2020-11-05 20:24 UTC (permalink / raw)
  To: Olga Kornievskaia
  Cc: Trond Myklebust, Anna Schumaker, Paul Moore, Stephen Smalley,
	linux-nfs, Linux Security Module list, SElinux list

On Thu, Nov 5, 2020 at 8:51 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > <olga.kornievskaia@gmail.com> wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > Currently, the client will always ask for security_labels if the server
> > > returns that it supports that feature regardless of any LSM modules
> > > (such as Selinux) enforcing security policy. This adds performance
> > > penalty to the READDIR operation.
> > >
> > > Instead, query the LSM module to find if anything is enabled and
> > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> >
> > Having spent some time staring at some of the NFS code very recently,
> > I can't help but suggest: Would it perhaps be enough to decide whether
> > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some LSM
> > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > filesystem to give it labels (or at least that's how I interpret the
> > cryptic name) [1]. It's just a shot in the dark, but it seems to fit
> > this use case.
> >
> > [1] https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
>
> Very interesting. I was not aware of something like that nor was it
> mentioned when I asked on the selinux mailing list. I wonder if this
> is a supported feature that will always stay? In that case, NFS
> wouldn't need the extra hook that was added for this series. I will
> try this out and report back.

I wish I could have suggested it sooner, but I only learned about it
in recent days while investigating a bug that involves SELinux and NFS
:) And when I saw your patch, it immediately reminded me of that flag.

I don't think it's going away, at least as long as NFS depends on it.
If someone were to remove it, they would have to provide something
equivalent to make sure no existing users get broken, as with any
in-kernel change.

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 19:51     ` Olga Kornievskaia
  2020-11-05 20:24       ` Ondrej Mosnacek
@ 2020-11-05 21:18       ` Trond Myklebust
  2020-11-05 21:43         ` Olga Kornievskaia
  1 sibling, 1 reply; 13+ messages in thread
From: Trond Myklebust @ 2020-11-05 21:18 UTC (permalink / raw)
  To: omosnace, olga.kornievskaia
  Cc: selinux, linux-nfs, linux-security-module, anna.schumaker, paul,
	stephen.smalley.work

On Thu, 2020-11-05 at 14:51 -0500, Olga Kornievskaia wrote:
> On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > 
> > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > <olga.kornievskaia@gmail.com> wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > > 
> > > Currently, the client will always ask for security_labels if the
> > > server
> > > returns that it supports that feature regardless of any LSM
> > > modules
> > > (such as Selinux) enforcing security policy. This adds
> > > performance
> > > penalty to the READDIR operation.
> > > 
> > > Instead, query the LSM module to find if anything is enabled and
> > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> > 
> > Having spent some time staring at some of the NFS code very
> > recently,
> > I can't help but suggest: Would it perhaps be enough to decide
> > whether
> > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some
> > LSM
> > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > filesystem to give it labels (or at least that's how I interpret
> > the
> > cryptic name) [1]. It's just a shot in the dark, but it seems to
> > fit
> > this use case.
> > 
> > [1]
> > https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
> 
> Very interesting. I was not aware of something like that nor was it
> mentioned when I asked on the selinux mailing list. I wonder if this
> is a supported feature that will always stay? In that case, NFS
> wouldn't need the extra hook that was added for this series. I will
> try this out and report back.

NFS_CAP_SECURITY_LABEL is just the NFS server capability flag. It tells
you whether or not the client believes that the server might support
NFSv4.2 requests for labelled NFS metadata.

My understanding of Olga's requirement is that she needs to be able to
ignore that flag and simply not query for labelled NFS metadata if the
NFS client is not configured to enforce the LSM policy. The objective
is to avoid unnecessary RPC traffic to the server to query for data
that is unused.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 21:18       ` Trond Myklebust
@ 2020-11-05 21:43         ` Olga Kornievskaia
  2020-11-06  8:47           ` Ondrej Mosnacek
  0 siblings, 1 reply; 13+ messages in thread
From: Olga Kornievskaia @ 2020-11-05 21:43 UTC (permalink / raw)
  To: Trond Myklebust
  Cc: omosnace, selinux, linux-nfs, linux-security-module,
	anna.schumaker, paul, stephen.smalley.work

On Thu, Nov 5, 2020 at 4:18 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Thu, 2020-11-05 at 14:51 -0500, Olga Kornievskaia wrote:
> > On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <omosnace@redhat.com>
> > wrote:
> > >
> > > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > > <olga.kornievskaia@gmail.com> wrote:
> > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > >
> > > > Currently, the client will always ask for security_labels if the
> > > > server
> > > > returns that it supports that feature regardless of any LSM
> > > > modules
> > > > (such as Selinux) enforcing security policy. This adds
> > > > performance
> > > > penalty to the READDIR operation.
> > > >
> > > > Instead, query the LSM module to find if anything is enabled and
> > > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> > >
> > > Having spent some time staring at some of the NFS code very
> > > recently,
> > > I can't help but suggest: Would it perhaps be enough to decide
> > > whether
> > > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some
> > > LSM
> > > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > > filesystem to give it labels (or at least that's how I interpret
> > > the
> > > cryptic name) [1]. It's just a shot in the dark, but it seems to
> > > fit
> > > this use case.
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
> >
> > Very interesting. I was not aware of something like that nor was it
> > mentioned when I asked on the selinux mailing list. I wonder if this
> > is a supported feature that will always stay? In that case, NFS
> > wouldn't need the extra hook that was added for this series. I will
> > try this out and report back.
>
> NFS_CAP_SECURITY_LABEL is just the NFS server capability flag. It tells
> you whether or not the client believes that the server might support
> NFSv4.2 requests for labelled NFS metadata.
>
> My understanding of Olga's requirement is that she needs to be able to
> ignore that flag and simply not query for labelled NFS metadata if the
> NFS client is not configured to enforce the LSM policy. The objective
> is to avoid unnecessary RPC traffic to the server to query for data
> that is unused.

Actually that seems to be working. I think it's because while the
server returns that it supports sec_labels, after calling
security_sb_set_mnt_opts() the kflags_out don't have this
SECURITY_LSM_NATIVE_LABELS set (assuming this happens because selinux
isn't enabled) then we turned server's sec_labl cap off.

I'm about to send v2 without relying on modifications to selinux.

>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 17:33 ` [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state Olga Kornievskaia
@ 2020-11-05 23:06     ` kernel test robot
  2020-11-05 23:06     ` kernel test robot
  1 sibling, 0 replies; 13+ messages in thread
From: kernel test robot @ 2020-11-05 23:06 UTC (permalink / raw)
  To: Olga Kornievskaia, trond.myklebust, anna.schumaker, paul,
	stephen.smalley.work
  Cc: kbuild-all, linux-nfs, linux-security-module, selinux

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

Hi Olga,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nfs/linux-next]
[also build test ERROR on pcmoore-selinux/next linus/master security/next-testing v5.10-rc2 next-20201105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: openrisc-randconfig-r002-20201104 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d765c00ede01a334b7a3f995ab27b8d4ebd5ea38
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
        git checkout d765c00ede01a334b7a3f995ab27b8d4ebd5ea38
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/nfs/nfs4proc.c: In function '_nfs4_proc_readdir':
>> fs/nfs/nfs4proc.c:4982:18: error: 'LSM_FQUERY_VFS_XATTRS' undeclared (first use in this function)
    4982 |  int sec_flags = LSM_FQUERY_VFS_XATTRS;
         |                  ^~~~~~~~~~~~~~~~~~~~~
   fs/nfs/nfs4proc.c:4982:18: note: each undeclared identifier is reported only once for each function it appears in
>> fs/nfs/nfs4proc.c:4987:7: error: implicit declaration of function 'security_func_query_vfs' [-Werror=implicit-function-declaration]
    4987 |  if (!security_func_query_vfs(sec_flags))
         |       ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/LSM_FQUERY_VFS_XATTRS +4982 fs/nfs/nfs4proc.c

  4960	
  4961	static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
  4962			u64 cookie, struct page **pages, unsigned int count, bool plus)
  4963	{
  4964		struct inode		*dir = d_inode(dentry);
  4965		struct nfs4_readdir_arg args = {
  4966			.fh = NFS_FH(dir),
  4967			.pages = pages,
  4968			.pgbase = 0,
  4969			.count = count,
  4970			.bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
  4971			.plus = plus,
  4972			.labels = true,
  4973		};
  4974		struct nfs4_readdir_res res;
  4975		struct rpc_message msg = {
  4976			.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_READDIR],
  4977			.rpc_argp = &args,
  4978			.rpc_resp = &res,
  4979			.rpc_cred = cred,
  4980		};
  4981		int			status;
> 4982		int sec_flags = LSM_FQUERY_VFS_XATTRS;
  4983	
  4984		dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
  4985				dentry,
  4986				(unsigned long long)cookie);
> 4987		if (!security_func_query_vfs(sec_flags))
  4988			args.labels = false;
  4989		nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
  4990		res.pgbase = args.pgbase;
  4991		status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
  4992		if (status >= 0) {
  4993			memcpy(NFS_I(dir)->cookieverf, res.verifier.data, NFS4_VERIFIER_SIZE);
  4994			status += args.pgbase;
  4995		}
  4996	
  4997		nfs_invalidate_atime(dir);
  4998	
  4999		dprintk("%s: returns %d\n", __func__, status);
  5000		return status;
  5001	}
  5002	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31537 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
@ 2020-11-05 23:06     ` kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2020-11-05 23:06 UTC (permalink / raw)
  To: kbuild-all

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

Hi Olga,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nfs/linux-next]
[also build test ERROR on pcmoore-selinux/next linus/master security/next-testing v5.10-rc2 next-20201105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: openrisc-randconfig-r002-20201104 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d765c00ede01a334b7a3f995ab27b8d4ebd5ea38
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
        git checkout d765c00ede01a334b7a3f995ab27b8d4ebd5ea38
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/nfs/nfs4proc.c: In function '_nfs4_proc_readdir':
>> fs/nfs/nfs4proc.c:4982:18: error: 'LSM_FQUERY_VFS_XATTRS' undeclared (first use in this function)
    4982 |  int sec_flags = LSM_FQUERY_VFS_XATTRS;
         |                  ^~~~~~~~~~~~~~~~~~~~~
   fs/nfs/nfs4proc.c:4982:18: note: each undeclared identifier is reported only once for each function it appears in
>> fs/nfs/nfs4proc.c:4987:7: error: implicit declaration of function 'security_func_query_vfs' [-Werror=implicit-function-declaration]
    4987 |  if (!security_func_query_vfs(sec_flags))
         |       ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/LSM_FQUERY_VFS_XATTRS +4982 fs/nfs/nfs4proc.c

  4960	
  4961	static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
  4962			u64 cookie, struct page **pages, unsigned int count, bool plus)
  4963	{
  4964		struct inode		*dir = d_inode(dentry);
  4965		struct nfs4_readdir_arg args = {
  4966			.fh = NFS_FH(dir),
  4967			.pages = pages,
  4968			.pgbase = 0,
  4969			.count = count,
  4970			.bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
  4971			.plus = plus,
  4972			.labels = true,
  4973		};
  4974		struct nfs4_readdir_res res;
  4975		struct rpc_message msg = {
  4976			.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_READDIR],
  4977			.rpc_argp = &args,
  4978			.rpc_resp = &res,
  4979			.rpc_cred = cred,
  4980		};
  4981		int			status;
> 4982		int sec_flags = LSM_FQUERY_VFS_XATTRS;
  4983	
  4984		dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
  4985				dentry,
  4986				(unsigned long long)cookie);
> 4987		if (!security_func_query_vfs(sec_flags))
  4988			args.labels = false;
  4989		nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
  4990		res.pgbase = args.pgbase;
  4991		status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
  4992		if (status >= 0) {
  4993			memcpy(NFS_I(dir)->cookieverf, res.verifier.data, NFS4_VERIFIER_SIZE);
  4994			status += args.pgbase;
  4995		}
  4996	
  4997		nfs_invalidate_atime(dir);
  4998	
  4999		dprintk("%s: returns %d\n", __func__, status);
  5000		return status;
  5001	}
  5002	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31537 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state
  2020-11-05 21:43         ` Olga Kornievskaia
@ 2020-11-06  8:47           ` Ondrej Mosnacek
  0 siblings, 0 replies; 13+ messages in thread
From: Ondrej Mosnacek @ 2020-11-06  8:47 UTC (permalink / raw)
  To: Olga Kornievskaia
  Cc: Trond Myklebust, selinux, linux-nfs, linux-security-module,
	anna.schumaker, paul, stephen.smalley.work

On Thu, Nov 5, 2020 at 10:43 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
> On Thu, Nov 5, 2020 at 4:18 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
> >
> > On Thu, 2020-11-05 at 14:51 -0500, Olga Kornievskaia wrote:
> > > On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <omosnace@redhat.com>
> > > wrote:
> > > >
> > > > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > > > <olga.kornievskaia@gmail.com> wrote:
> > > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > > >
> > > > > Currently, the client will always ask for security_labels if the
> > > > > server
> > > > > returns that it supports that feature regardless of any LSM
> > > > > modules
> > > > > (such as Selinux) enforcing security policy. This adds
> > > > > performance
> > > > > penalty to the READDIR operation.
> > > > >
> > > > > Instead, query the LSM module to find if anything is enabled and
> > > > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> > > >
> > > > Having spent some time staring at some of the NFS code very
> > > > recently,
> > > > I can't help but suggest: Would it perhaps be enough to decide
> > > > whether
> > > > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > > > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some
> > > > LSM
> > > > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > > > filesystem to give it labels (or at least that's how I interpret
> > > > the
> > > > cryptic name) [1]. It's just a shot in the dark, but it seems to
> > > > fit
> > > > this use case.
> > > >
> > > > [1]
> > > > https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
> > >
> > > Very interesting. I was not aware of something like that nor was it
> > > mentioned when I asked on the selinux mailing list. I wonder if this
> > > is a supported feature that will always stay? In that case, NFS
> > > wouldn't need the extra hook that was added for this series. I will
> > > try this out and report back.
> >
> > NFS_CAP_SECURITY_LABEL is just the NFS server capability flag. It tells
> > you whether or not the client believes that the server might support
> > NFSv4.2 requests for labelled NFS metadata.
> >
> > My understanding of Olga's requirement is that she needs to be able to
> > ignore that flag and simply not query for labelled NFS metadata if the
> > NFS client is not configured to enforce the LSM policy. The objective
> > is to avoid unnecessary RPC traffic to the server to query for data
> > that is unused.
>
> Actually that seems to be working. I think it's because while the
> server returns that it supports sec_labels, after calling
> security_sb_set_mnt_opts() the kflags_out don't have this
> SECURITY_LSM_NATIVE_LABELS set (assuming this happens because selinux
> isn't enabled) then we turned server's sec_labl cap off.
>
> I'm about to send v2 without relying on modifications to selinux.

Just to keep the LSM and SELinux lists in the loop: a v2 has been
posted to linux-nfs that uses NFS_CAP_SECURITY_LABEL instead of adding
a new hook:
https://lore.kernel.org/linux-nfs/CAFqZXNtJ2fkae7Xt-+nDR79kVs=yY=9vSoZaS-V-5UKSk22s4A@mail.gmail.com/T/

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality
  2020-11-05 17:33 [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Olga Kornievskaia
  2020-11-05 17:33 ` [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state Olga Kornievskaia
  2020-11-05 19:39 ` [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Casey Schaufler
@ 2020-11-07  1:33 ` James Morris
  2020-11-14 10:12 ` kernel test robot
  3 siblings, 0 replies; 13+ messages in thread
From: James Morris @ 2020-11-07  1:33 UTC (permalink / raw)
  To: Olga Kornievskaia
  Cc: trond.myklebust, anna.schumaker, paul, stephen.smalley.work,
	linux-nfs, linux-security-module, selinux

On Thu, 5 Nov 2020, Olga Kornievskaia wrote:

> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Add a new hook func_query_vfs to query an LSM module (such as
> SELinux) with the intention of finding whether or not it is enabled
> or perhaps supports some functionality.
> 
> NFS stores security labels for file system objects and SElinux
> or any other LSM module can query those objects. But it's
> wasteful to do so when no security enforcement is taking place.
> Using a new API call of security_func_query_vfs() and asking if

Seems we lost something here.



-- 
James Morris
<jmorris@namei.org>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality
  2020-11-05 17:33 [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Olga Kornievskaia
                   ` (2 preceding siblings ...)
  2020-11-07  1:33 ` James Morris
@ 2020-11-14 10:12 ` kernel test robot
  3 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2020-11-14 10:12 UTC (permalink / raw)
  To: kbuild-all

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

Hi Olga,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nfs/linux-next]
[also build test ERROR on pcmoore-selinux/next linus/master security/next-testing v5.10-rc3 next-20201113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/0a59499b0fcef78631c9a8619f330ae47802742f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
        git checkout 0a59499b0fcef78631c9a8619f330ae47802742f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   include/linux/lsm_hook_defs.h: In function 'bpf_lsm_func_query_vfs':
>> include/linux/lsm_hook_defs.h:260:34: error: parameter name omitted
     260 | LSM_HOOK(int, 0, func_query_vfs, unsigned int)
         |                                  ^~~~~~~~~~~~
   kernel/bpf/bpf_lsm.c:21:29: note: in definition of macro 'LSM_HOOK'
      21 | noinline RET bpf_lsm_##NAME(__VA_ARGS__) \
         |                             ^~~~~~~~~~~

vim +260 include/linux/lsm_hook_defs.h

   107	
   108	/* Needed for inode based security check */
   109	LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
   110		 unsigned int obj_type)
   111	LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
   112	LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
   113	LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
   114		 struct inode *dir, const struct qstr *qstr, const char **name,
   115		 void **value, size_t *len)
   116	LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
   117		 umode_t mode)
   118	LSM_HOOK(int, 0, inode_link, struct dentry *old_dentry, struct inode *dir,
   119		 struct dentry *new_dentry)
   120	LSM_HOOK(int, 0, inode_unlink, struct inode *dir, struct dentry *dentry)
   121	LSM_HOOK(int, 0, inode_symlink, struct inode *dir, struct dentry *dentry,
   122		 const char *old_name)
   123	LSM_HOOK(int, 0, inode_mkdir, struct inode *dir, struct dentry *dentry,
   124		 umode_t mode)
   125	LSM_HOOK(int, 0, inode_rmdir, struct inode *dir, struct dentry *dentry)
   126	LSM_HOOK(int, 0, inode_mknod, struct inode *dir, struct dentry *dentry,
   127		 umode_t mode, dev_t dev)
   128	LSM_HOOK(int, 0, inode_rename, struct inode *old_dir, struct dentry *old_dentry,
   129		 struct inode *new_dir, struct dentry *new_dentry)
   130	LSM_HOOK(int, 0, inode_readlink, struct dentry *dentry)
   131	LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
   132		 bool rcu)
   133	LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
   134	LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
   135	LSM_HOOK(int, 0, inode_getattr, const struct path *path)
   136	LSM_HOOK(int, 0, inode_setxattr, struct dentry *dentry, const char *name,
   137		 const void *value, size_t size, int flags)
   138	LSM_HOOK(void, LSM_RET_VOID, inode_post_setxattr, struct dentry *dentry,
   139		 const char *name, const void *value, size_t size, int flags)
   140	LSM_HOOK(int, 0, inode_getxattr, struct dentry *dentry, const char *name)
   141	LSM_HOOK(int, 0, inode_listxattr, struct dentry *dentry)
   142	LSM_HOOK(int, 0, inode_removexattr, struct dentry *dentry, const char *name)
   143	LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry)
   144	LSM_HOOK(int, 0, inode_killpriv, struct dentry *dentry)
   145	LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct inode *inode,
   146		 const char *name, void **buffer, bool alloc)
   147	LSM_HOOK(int, -EOPNOTSUPP, inode_setsecurity, struct inode *inode,
   148		 const char *name, const void *value, size_t size, int flags)
   149	LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
   150		 size_t buffer_size)
   151	LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
   152	LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
   153	LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, const char *name)
   154	LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
   155		 struct kernfs_node *kn)
   156	LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
   157	LSM_HOOK(int, 0, file_alloc_security, struct file *file)
   158	LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
   159	LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
   160		 unsigned long arg)
   161	LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
   162	LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
   163		 unsigned long prot, unsigned long flags)
   164	LSM_HOOK(int, 0, file_mprotect, struct vm_area_struct *vma,
   165		 unsigned long reqprot, unsigned long prot)
   166	LSM_HOOK(int, 0, file_lock, struct file *file, unsigned int cmd)
   167	LSM_HOOK(int, 0, file_fcntl, struct file *file, unsigned int cmd,
   168		 unsigned long arg)
   169	LSM_HOOK(void, LSM_RET_VOID, file_set_fowner, struct file *file)
   170	LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
   171		 struct fown_struct *fown, int sig)
   172	LSM_HOOK(int, 0, file_receive, struct file *file)
   173	LSM_HOOK(int, 0, file_open, struct file *file)
   174	LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
   175		 unsigned long clone_flags)
   176	LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
   177	LSM_HOOK(int, 0, cred_alloc_blank, struct cred *cred, gfp_t gfp)
   178	LSM_HOOK(void, LSM_RET_VOID, cred_free, struct cred *cred)
   179	LSM_HOOK(int, 0, cred_prepare, struct cred *new, const struct cred *old,
   180		 gfp_t gfp)
   181	LSM_HOOK(void, LSM_RET_VOID, cred_transfer, struct cred *new,
   182		 const struct cred *old)
   183	LSM_HOOK(void, LSM_RET_VOID, cred_getsecid, const struct cred *c, u32 *secid)
   184	LSM_HOOK(int, 0, kernel_act_as, struct cred *new, u32 secid)
   185	LSM_HOOK(int, 0, kernel_create_files_as, struct cred *new, struct inode *inode)
   186	LSM_HOOK(int, 0, kernel_module_request, char *kmod_name)
   187	LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id, bool contents)
   188	LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
   189		 enum kernel_load_data_id id, char *description)
   190	LSM_HOOK(int, 0, kernel_read_file, struct file *file,
   191		 enum kernel_read_file_id id, bool contents)
   192	LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
   193		 loff_t size, enum kernel_read_file_id id)
   194	LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
   195		 int flags)
   196	LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old,
   197		 int flags)
   198	LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid)
   199	LSM_HOOK(int, 0, task_getpgid, struct task_struct *p)
   200	LSM_HOOK(int, 0, task_getsid, struct task_struct *p)
   201	LSM_HOOK(void, LSM_RET_VOID, task_getsecid, struct task_struct *p, u32 *secid)
   202	LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice)
   203	LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio)
   204	LSM_HOOK(int, 0, task_getioprio, struct task_struct *p)
   205	LSM_HOOK(int, 0, task_prlimit, const struct cred *cred,
   206		 const struct cred *tcred, unsigned int flags)
   207	LSM_HOOK(int, 0, task_setrlimit, struct task_struct *p, unsigned int resource,
   208		 struct rlimit *new_rlim)
   209	LSM_HOOK(int, 0, task_setscheduler, struct task_struct *p)
   210	LSM_HOOK(int, 0, task_getscheduler, struct task_struct *p)
   211	LSM_HOOK(int, 0, task_movememory, struct task_struct *p)
   212	LSM_HOOK(int, 0, task_kill, struct task_struct *p, struct kernel_siginfo *info,
   213		 int sig, const struct cred *cred)
   214	LSM_HOOK(int, -ENOSYS, task_prctl, int option, unsigned long arg2,
   215		 unsigned long arg3, unsigned long arg4, unsigned long arg5)
   216	LSM_HOOK(void, LSM_RET_VOID, task_to_inode, struct task_struct *p,
   217		 struct inode *inode)
   218	LSM_HOOK(int, 0, ipc_permission, struct kern_ipc_perm *ipcp, short flag)
   219	LSM_HOOK(void, LSM_RET_VOID, ipc_getsecid, struct kern_ipc_perm *ipcp,
   220		 u32 *secid)
   221	LSM_HOOK(int, 0, msg_msg_alloc_security, struct msg_msg *msg)
   222	LSM_HOOK(void, LSM_RET_VOID, msg_msg_free_security, struct msg_msg *msg)
   223	LSM_HOOK(int, 0, msg_queue_alloc_security, struct kern_ipc_perm *perm)
   224	LSM_HOOK(void, LSM_RET_VOID, msg_queue_free_security,
   225		 struct kern_ipc_perm *perm)
   226	LSM_HOOK(int, 0, msg_queue_associate, struct kern_ipc_perm *perm, int msqflg)
   227	LSM_HOOK(int, 0, msg_queue_msgctl, struct kern_ipc_perm *perm, int cmd)
   228	LSM_HOOK(int, 0, msg_queue_msgsnd, struct kern_ipc_perm *perm,
   229		 struct msg_msg *msg, int msqflg)
   230	LSM_HOOK(int, 0, msg_queue_msgrcv, struct kern_ipc_perm *perm,
   231		 struct msg_msg *msg, struct task_struct *target, long type, int mode)
   232	LSM_HOOK(int, 0, shm_alloc_security, struct kern_ipc_perm *perm)
   233	LSM_HOOK(void, LSM_RET_VOID, shm_free_security, struct kern_ipc_perm *perm)
   234	LSM_HOOK(int, 0, shm_associate, struct kern_ipc_perm *perm, int shmflg)
   235	LSM_HOOK(int, 0, shm_shmctl, struct kern_ipc_perm *perm, int cmd)
   236	LSM_HOOK(int, 0, shm_shmat, struct kern_ipc_perm *perm, char __user *shmaddr,
   237		 int shmflg)
   238	LSM_HOOK(int, 0, sem_alloc_security, struct kern_ipc_perm *perm)
   239	LSM_HOOK(void, LSM_RET_VOID, sem_free_security, struct kern_ipc_perm *perm)
   240	LSM_HOOK(int, 0, sem_associate, struct kern_ipc_perm *perm, int semflg)
   241	LSM_HOOK(int, 0, sem_semctl, struct kern_ipc_perm *perm, int cmd)
   242	LSM_HOOK(int, 0, sem_semop, struct kern_ipc_perm *perm, struct sembuf *sops,
   243		 unsigned nsops, int alter)
   244	LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
   245	LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
   246		 struct inode *inode)
   247	LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, char *name,
   248		 char **value)
   249	LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
   250	LSM_HOOK(int, 0, ismaclabel, const char *name)
   251	LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata,
   252		 u32 *seclen)
   253	LSM_HOOK(int, 0, secctx_to_secid, const char *secdata, u32 seclen, u32 *secid)
   254	LSM_HOOK(void, LSM_RET_VOID, release_secctx, char *secdata, u32 seclen)
   255	LSM_HOOK(void, LSM_RET_VOID, inode_invalidate_secctx, struct inode *inode)
   256	LSM_HOOK(int, 0, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
   257	LSM_HOOK(int, 0, inode_setsecctx, struct dentry *dentry, void *ctx, u32 ctxlen)
   258	LSM_HOOK(int, 0, inode_getsecctx, struct inode *inode, void **ctx,
   259		 u32 *ctxlen)
 > 260	LSM_HOOK(int, 0, func_query_vfs, unsigned int)
   261	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 69039 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-11-14 10:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 17:33 [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Olga Kornievskaia
2020-11-05 17:33 ` [PATCH 2/2] NFSv4.2: condition READDIR's mask for security label based on LSM state Olga Kornievskaia
2020-11-05 18:55   ` Ondrej Mosnacek
2020-11-05 19:51     ` Olga Kornievskaia
2020-11-05 20:24       ` Ondrej Mosnacek
2020-11-05 21:18       ` Trond Myklebust
2020-11-05 21:43         ` Olga Kornievskaia
2020-11-06  8:47           ` Ondrej Mosnacek
2020-11-05 23:06   ` kernel test robot
2020-11-05 23:06     ` kernel test robot
2020-11-05 19:39 ` [PATCH 1/2] [lsm] introduce a new hook to query LSM for functionality Casey Schaufler
2020-11-07  1:33 ` James Morris
2020-11-14 10:12 ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.