[PATCH v2 0/5] RFC: Linux IMA on NFS prototype

[PATCH v2 0/5] RFC: Linux IMA on NFS prototype

[Chuck Lever]

This series implements support for accessing and updating the
security.ima xattr on files that reside on an NFS export. Since the
NFS protocol does not have capabilities like CAP_SYS_ADMIN, on NFS
clients, only root is allowed to set this xattr.

I'm interested in comments on the implementation, test results, or a
discussion of whether this proposal creates undesirable security
exposures.

Git repo: git://git.linux-nfs.org/projects/cel/cel-2.6.git

in the nfs-ima-prototype topic branch.


Implementation Notes

Please see the individual patch descriptions: standards action is
still required to define the official FATTR4 flag that all NFSv4.2
implementations recognize as meaning "the security.ima xattr". This
prototype is not guaranteed to interoperate with future prototypes
or standards-compliant implementations of this feature. It is for
experimental purposes only.

EVM is not supported in this prototype. The NFS protocol does not
support several of the xattrs that are protected by EVM: SMACK64,
Posix ACLs, and Linux file capabilities are not supported. When
these are present in an EVM hash, NFS clients can't retrieve them
to verify the hash.

This prototype does not match what is described in draft-ietf-nfsv4-
integrity-measurement. Since that draft was submitted, there has
been vigorous discussion on nfsv4@ietf.org about how the NFS
protocol should support Linux IMA. The prototype attempts a narrow
interpretation of what the comments have requested. The draft will
be updated to reflect the prototype implementation.


Changes since v1:
- Rebased on kernel v5.0
- Moved NFSD support out from behind CONFIG_NFSD_V4_SECURITY_LABELS
- Added a patch to remove ima_file_check call in NFSD

---

Chuck Lever (5):
      NFS: Define common IMA-related protocol elements
      NFSD: Prototype support for IMA on NFS (server)
      NFSD: Remove ima_file_check call
      NFS: Rename security xattr handler
      NFS: Prototype support for IMA on NFS (client)


 fs/nfs/nfs4_fs.h          |    1 
 fs/nfs/nfs4proc.c         |  134 +++++++++++++++++++++++++++++---
 fs/nfs/nfs4xdr.c          |  186 +++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfs4proc.c        |    9 ++
 fs/nfsd/nfs4xdr.c         |   49 ++++++++++--
 fs/nfsd/nfsd.h            |    3 -
 fs/nfsd/vfs.c             |   25 +++++-
 fs/nfsd/vfs.h             |    3 +
 fs/nfsd/xdr4.h            |    3 +
 fs/xattr.c                |   25 +++---
 include/linux/nfs4.h      |    5 +
 include/linux/nfs_fs_sb.h |    1 
 include/linux/nfs_xdr.h   |   21 +++++
 13 files changed, 426 insertions(+), 39 deletions(-)

--
Chuck Lever

[PATCH v2 1/5] NFS: Define common IMA-related protocol elements

[Chuck Lever]

Common protocol definitions used by the server and client
implementations. A separate patch also makes it easier to identify
the code that needs to change when the flag name and value is
eventually standardized.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/nfs4.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 1b06f0b..ca3adb1 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -41,6 +41,7 @@ struct nfs4_acl {
 };
 
 #define NFS4_MAXLABELLEN	2048
+#define NFS4_MAXIMALEN		(4096)
 
 struct nfs4_label {
 	uint32_t	lfs;
@@ -451,6 +452,7 @@ enum change_attr_type4 {
 #define FATTR4_WORD2_CHANGE_ATTR_TYPE	(1UL << 15)
 #define FATTR4_WORD2_SECURITY_LABEL     (1UL << 16)
 #define FATTR4_WORD2_MODE_UMASK		(1UL << 17)
+#define FATTR4_WORD2_LINUX_IMA		(1UL << 18)
 
 /* MDS threshold bitmap bits */
 #define THRESHOLD_RD                    (1UL << 0)

[PATCH v2 2/5] NFSD: Prototype support for IMA on NFS (server)

[Chuck Lever]

Provide code to handle the "security.ima" xattr. The NFS server
converts incoming GETATTR and SETATTR calls to acesses and updates
of the xattr. Even if CONFIG_IMA is disabled, the NFS server can
access and update that extended attribute.

The new FATTR4 bit is made up; meaning we still have to go through a
standards process to allocate a bit that all NFS vendors agree on.
Thus there is no guarantee this prototype will interoperate with
others or with a future standards-based implementation.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c |    9 +++++++++
 fs/nfsd/nfs4xdr.c  |   49 +++++++++++++++++++++++++++++++++++++++++++------
 fs/nfsd/nfsd.h     |    3 ++-
 fs/nfsd/vfs.c      |   19 +++++++++++++++++++
 fs/nfsd/vfs.h      |    3 +++
 fs/nfsd/xdr4.h     |    3 +++
 fs/xattr.c         |   25 +++++++++++++------------
 7 files changed, 92 insertions(+), 19 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0cfd257..8851bce 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -979,6 +979,11 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 				&setattr->sa_label);
 	if (status)
 		goto out;
+	if (setattr->sa_ima.len)
+		status = nfsd4_set_ima_metadata(rqstp, &cstate->current_fh,
+						&setattr->sa_ima);
+	if (status)
+		goto out;
 	status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr,
 				0, (time_t)0);
 out:
@@ -2135,6 +2140,10 @@ static inline u32 nfsd4_getattr_rsize(struct svc_rqst *rqstp,
 		ret += NFS4_MAXLABELLEN + 12;
 		bmap2 &= ~FATTR4_WORD2_SECURITY_LABEL;
 	}
+	if (bmap2 & FATTR4_WORD2_LINUX_IMA) {
+		ret += NFS4_MAXIMALEN + 4;
+		bmap2 &= ~FATTR4_WORD2_LINUX_IMA;
+	}
 	/*
 	 * Largest of remaining attributes are 16 bytes (e.g.,
 	 * supported_attributes)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 3de42a7..3e8d90c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -39,6 +39,7 @@
 #include <linux/statfs.h>
 #include <linux/utsname.h>
 #include <linux/pagemap.h>
+#include <linux/xattr.h>
 #include <linux/sunrpc/svcauth_gss.h>
 
 #include "idmap.h"
@@ -318,7 +319,8 @@ static char *savemem(struct nfsd4_compoundargs *argp, __be32 *p, int nbytes)
 static __be32
 nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		   struct iattr *iattr, struct nfs4_acl **acl,
-		   struct xdr_netobj *label, int *umask)
+		   struct xdr_netobj *label, int *umask,
+		   struct xdr_netobj *ima)
 {
 	struct timespec ts;
 	int expected_len, len = 0;
@@ -455,7 +457,6 @@ static char *savemem(struct nfsd4_compoundargs *argp, __be32 *p, int nbytes)
 			goto xdr_error;
 		}
 	}
-
 	label->len = 0;
 	if (IS_ENABLED(CONFIG_NFSD_V4_SECURITY_LABEL) &&
 	    bmval[2] & FATTR4_WORD2_SECURITY_LABEL) {
@@ -489,6 +490,21 @@ static char *savemem(struct nfsd4_compoundargs *argp, __be32 *p, int nbytes)
 		*umask = dummy32 & S_IRWXUGO;
 		iattr->ia_valid |= ATTR_MODE;
 	}
+	ima->len = 0;
+	if (bmval[2] & FATTR4_WORD2_LINUX_IMA) {
+		READ_BUF(4);
+		len += 4;
+		dummy32 = be32_to_cpup(p++);
+		READ_BUF(dummy32);
+		if (dummy32 > NFS4_MAXIMALEN)
+			return nfserr_badlabel;
+		len += (XDR_QUADLEN(dummy32) << 2);
+		READMEM(buf, dummy32);
+		ima->len = dummy32;
+		ima->data = svcxdr_dupstr(argp, buf, dummy32);
+		if (!ima->data)
+			return nfserr_jukebox;
+	}
 	if (len != expected_len)
 		goto xdr_error;
 
@@ -684,7 +700,7 @@ static __be32 nfsd4_decode_bind_conn_to_session(struct nfsd4_compoundargs *argp,
 
 	status = nfsd4_decode_fattr(argp, create->cr_bmval, &create->cr_iattr,
 				    &create->cr_acl, &create->cr_label,
-				    &create->cr_umask);
+				    &create->cr_umask, &create->cr_ima);
 	if (status)
 		goto out;
 
@@ -936,7 +952,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
 		case NFS4_CREATE_GUARDED:
 			status = nfsd4_decode_fattr(argp, open->op_bmval,
 				&open->op_iattr, &open->op_acl, &open->op_label,
-				&open->op_umask);
+				&open->op_umask, &open->op_ima);
 			if (status)
 				goto out;
 			break;
@@ -951,7 +967,7 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
 			COPYMEM(open->op_verf.data, NFS4_VERIFIER_SIZE);
 			status = nfsd4_decode_fattr(argp, open->op_bmval,
 				&open->op_iattr, &open->op_acl, &open->op_label,
-				&open->op_umask);
+				&open->op_umask, &open->op_ima);
 			if (status)
 				goto out;
 			break;
@@ -1188,7 +1204,8 @@ static __be32 nfsd4_decode_opaque(struct nfsd4_compoundargs *argp, struct xdr_ne
 	if (status)
 		return status;
 	return nfsd4_decode_fattr(argp, setattr->sa_bmval, &setattr->sa_iattr,
-				  &setattr->sa_acl, &setattr->sa_label, NULL);
+				  &setattr->sa_acl, &setattr->sa_label, NULL,
+				  &setattr->sa_ima);
 }
 
 static __be32
@@ -2430,6 +2447,7 @@ static int get_parent_attributes(struct svc_export *exp, struct kstat *stat)
 		.dentry	= dentry,
 	};
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
+	struct xdr_netobj ima = { 0, NULL };
 
 	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
 	BUG_ON(!nfsd_attrs_supported(minorversion, bmval));
@@ -2491,6 +2509,16 @@ static int get_parent_attributes(struct svc_export *exp, struct kstat *stat)
 	}
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
 
+	if (bmval2 & FATTR4_WORD2_LINUX_IMA) {
+		err = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA,
+					 (char **)&ima.data, 0, GFP_KERNEL);
+		if (err == -ENODATA)
+			bmval2 &= ~FATTR4_WORD2_LINUX_IMA;
+		else if (err < 0)
+			goto out_nfserr;
+		ima.len = err;
+	}
+
 	status = nfsd4_encode_bitmap(xdr, bmval0, bmval1, bmval2);
 	if (status)
 		goto out;
@@ -2913,6 +2941,14 @@ static int get_parent_attributes(struct svc_export *exp, struct kstat *stat)
 			goto out;
 	}
 
+	if (bmval2 & FATTR4_WORD2_LINUX_IMA) {
+		p = xdr_reserve_space(xdr,
+				      sizeof(__be32) + xdr_align_size(ima.len));
+		if (!p)
+			goto out_resource;
+		xdr_encode_netobj(p, &ima);
+	}
+
 	attrlen = htonl(xdr->buf->len - attrlen_offset - 4);
 	write_bytes_to_xdr_buf(xdr->buf, attrlen_offset, &attrlen, 4);
 	status = nfs_ok;
@@ -2922,6 +2958,7 @@ static int get_parent_attributes(struct svc_export *exp, struct kstat *stat)
 	if (context)
 		security_release_secctx(context, contextlen);
 #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
+	kfree(ima.data);
 	kfree(acl);
 	if (tempfh) {
 		fh_put(tempfh);
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 0668999..c87ff8e 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -362,6 +362,7 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
 	(NFSD4_1_SUPPORTED_ATTRS_WORD2 | \
 	FATTR4_WORD2_CHANGE_ATTR_TYPE | \
 	FATTR4_WORD2_MODE_UMASK | \
+	FATTR4_WORD2_LINUX_IMA | \
 	NFSD4_2_SECURITY_ATTRS)
 
 extern const u32 nfsd_suppattrs[3][3];
@@ -399,7 +400,7 @@ static inline bool nfsd_attrs_supported(u32 minorversion, const u32 *bmval)
 #define MAYBE_FATTR4_WORD2_SECURITY_LABEL 0
 #endif
 #define NFSD_WRITEABLE_ATTRS_WORD2 \
-	(FATTR4_WORD2_MODE_UMASK \
+	(FATTR4_WORD2_MODE_UMASK | FATTR4_WORD2_LINUX_IMA \
 	| MAYBE_FATTR4_WORD2_SECURITY_LABEL)
 
 #define NFSD_SUPPATTR_EXCLCREAT_WORD0 \
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 7dc98e1..3c00072 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -551,6 +551,25 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
 }
 #endif
 
+__be32 nfsd4_set_ima_metadata(struct svc_rqst *rqstp, struct svc_fh *fhp,
+			      struct xdr_netobj *ima)
+{
+	struct dentry *dentry;
+	int host_error;
+	__be32 error;
+
+	error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, NFSD_MAY_SATTR);
+	if (error)
+		return error;
+	dentry = fhp->fh_dentry;
+
+	inode_lock(d_inode(dentry));
+	host_error = __vfs_setxattr_noperm(dentry, XATTR_NAME_IMA, ima->data,
+					   ima->len, 0);
+	inode_unlock(d_inode(dentry));
+	return nfserrno(host_error);
+}
+
 __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
 		u64 dst_pos, u64 count)
 {
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index a7e1073..acfc2b0 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -55,6 +55,9 @@ __be32		nfsd_setattr(struct svc_rqst *, struct svc_fh *,
 #ifdef CONFIG_NFSD_V4
 __be32          nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
 		    struct xdr_netobj *);
+__be32		nfsd4_set_ima_metadata(struct svc_rqst *rqstp,
+				       struct svc_fh *fhp,
+				       struct xdr_netobj *ima);
 __be32		nfsd4_vfs_fallocate(struct svc_rqst *, struct svc_fh *,
 				    struct file *, loff_t, loff_t, int);
 __be32		nfsd4_clone_file_range(struct file *, u64, struct file *,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index feeb6d4..2f3307f 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -123,6 +123,7 @@ struct nfsd4_create {
 	struct nfsd4_change_info  cr_cinfo; /* response */
 	struct nfs4_acl *cr_acl;
 	struct xdr_netobj cr_label;
+	struct xdr_netobj cr_ima;
 };
 #define cr_datalen	u.link.datalen
 #define cr_data		u.link.data
@@ -255,6 +256,7 @@ struct nfsd4_open {
 	struct nfs4_clnt_odstate *op_odstate; /* used during processing */
 	struct nfs4_acl *op_acl;
 	struct xdr_netobj op_label;
+	struct xdr_netobj op_ima;
 };
 
 struct nfsd4_open_confirm {
@@ -339,6 +341,7 @@ struct nfsd4_setattr {
 	struct iattr	sa_iattr;           /* request */
 	struct nfs4_acl *sa_acl;
 	struct xdr_netobj sa_label;
+	struct xdr_netobj sa_ima;
 };
 
 struct nfsd4_setclientid {
diff --git a/fs/xattr.c b/fs/xattr.c
index 0d6a6a4..5674be1 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -151,20 +151,20 @@
 EXPORT_SYMBOL(__vfs_setxattr);
 
 /**
- *  __vfs_setxattr_noperm - perform setxattr operation without performing
- *  permission checks.
+ * __vfs_setxattr_noperm - perform setxattr operation without performing
+ * permission checks.
  *
- *  @dentry - object to perform setxattr on
- *  @name - xattr name to set
- *  @value - value to set @name to
- *  @size - size of @value
- *  @flags - flags to pass into filesystem operations
+ * @dentry: object to perform setxattr on
+ * @name: xattr name to set
+ * @value: value to set @name to
+ * @size: size of @value
+ * @flags: flags to pass into filesystem operations
  *
- *  returns the result of the internal setxattr or setsecurity operations.
+ * Returns the result of the internal setxattr or setsecurity operations.
  *
- *  This function requires the caller to lock the inode's i_mutex before it
- *  is executed. It also assumes that the caller will make the appropriate
- *  permission checks.
+ * This function requires the caller to lock the inode's i_mutex before it
+ * is executed. It also assumes that the caller will make the appropriate
+ * permission checks.
  */
 int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 		const void *value, size_t size, int flags)
@@ -202,7 +202,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 
 	return error;
 }
-
+EXPORT_SYMBOL_GPL(__vfs_setxattr_noperm);
 
 int
 vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
@@ -295,6 +295,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 	*xattr_value = value;
 	return error;
 }
+EXPORT_SYMBOL_GPL(vfs_getxattr_alloc);
 
 ssize_t
 __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,

[PATCH v2 3/5] NFSD: Remove ima_file_check call

[Chuck Lever]

The NFS server needs to allow NFS clients to perform their own
attestation and measurement.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/vfs.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 3c00072..524c6e5 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
 		goto out_nfserr;
 	}
 
-	host_err = ima_file_check(file, may_flags);
-	if (host_err) {
-		fput(file);
-		goto out_nfserr;
-	}
-
 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
 		file->f_mode |= FMODE_64BITHASH;
 	else

[PATCH v2 4/5] NFS: Rename security xattr handler

[Chuck Lever]

Refactor: In a moment, the handler will no longer deal with only
NFSv4 security labels. Give it a more generic name.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4proc.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 557a5d6..df0ee42 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7128,20 +7128,19 @@ static bool nfs4_xattr_list_nfs4_acl(struct dentry *dentry)
 
 #ifdef CONFIG_NFS_V4_SECURITY_LABEL
 
-static int nfs4_xattr_set_nfs4_label(const struct xattr_handler *handler,
-				     struct dentry *unused, struct inode *inode,
-				     const char *key, const void *buf,
-				     size_t buflen, int flags)
+static int nfs4_xattr_set_security(const struct xattr_handler *handler,
+				   struct dentry *unused, struct inode *inode,
+				   const char *key, const void *buf,
+				   size_t buflen, int flags)
 {
 	if (security_ismaclabel(key))
 		return nfs4_set_security_label(inode, buf, buflen);
-
 	return -EOPNOTSUPP;
 }
 
-static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
-				     struct dentry *unused, struct inode *inode,
-				     const char *key, void *buf, size_t buflen)
+static int nfs4_xattr_get_security(const struct xattr_handler *handler,
+				   struct dentry *unused, struct inode *inode,
+				   const char *key, void *buf, size_t buflen)
 {
 	if (security_ismaclabel(key))
 		return nfs4_get_security_label(inode, buf, buflen);
@@ -7161,10 +7160,10 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
 	return len;
 }
 
-static const struct xattr_handler nfs4_xattr_nfs4_label_handler = {
+static const struct xattr_handler nfs4_xattr_security_handler = {
 	.prefix = XATTR_SECURITY_PREFIX,
-	.get	= nfs4_xattr_get_nfs4_label,
-	.set	= nfs4_xattr_set_nfs4_label,
+	.get	= nfs4_xattr_get_security,
+	.set	= nfs4_xattr_set_security,
 };
 
 #else
@@ -9824,7 +9823,7 @@ static ssize_t nfs4_listxattr(struct dentry *dentry, char *list, size_t size)
 const struct xattr_handler *nfs4_xattr_handlers[] = {
 	&nfs4_xattr_nfs4_acl_handler,
 #ifdef CONFIG_NFS_V4_SECURITY_LABEL
-	&nfs4_xattr_nfs4_label_handler,
+	&nfs4_xattr_security_handler,
 #endif
 	NULL
 };

[PATCH v2 5/5] NFS: Prototype support for IMA on NFS (client)

[Chuck Lever]

When NFSv4 Security Label support is enabled and kernel Integrity
and IMA support is enabled (via CONFIG), then build in code to
handle the "security.ima" xattr. The NFS client converts this to
a particular FATTR4 bit which can be handled on the wire via a
standard NFSv4 GETATTR or SETATTR operation.

The new FATTR4 bit is made up; meaning we still have to go through a
standards process to allocate a bit that all NFS vendors agree on.
Thus there is no guarantee this prototype will interoperate with
others or with a future standards-based implementation.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4_fs.h          |    1 
 fs/nfs/nfs4proc.c         |  111 ++++++++++++++++++++++++++-
 fs/nfs/nfs4xdr.c          |  186 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/nfs4.h      |    3 +
 include/linux/nfs_fs_sb.h |    1 
 include/linux/nfs_xdr.h   |   21 +++++
 6 files changed, 321 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 06ac3d9a..795f2f5 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -424,6 +424,7 @@ extern int nfs4_detect_session_trunking(struct nfs_client *clp,
 extern const u32 nfs4_pathconf_bitmap[3];
 extern const u32 nfs4_fsinfo_bitmap[3];
 extern const u32 nfs4_fs_locations_bitmap[3];
+extern const u32 nfs4_ima_bitmap[3];
 
 void nfs40_shutdown_client(struct nfs_client *);
 void nfs41_shutdown_client(struct nfs_client *);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index df0ee42..fc4cb55 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -205,6 +205,9 @@ static int nfs4_map_errors(int err)
 	| FATTR4_WORD1_MOUNTED_ON_FILEID,
 #ifdef CONFIG_NFS_V4_SECURITY_LABEL
 	FATTR4_WORD2_SECURITY_LABEL
+#ifdef CONFIG_IMA
+	| FATTR4_WORD2_LINUX_IMA
+#endif
 #endif
 };
 
@@ -275,6 +278,8 @@ static int nfs4_map_errors(int err)
 	| FATTR4_WORD1_MOUNTED_ON_FILEID,
 };
 
+const u32 nfs4_ima_bitmap[3] = { 0, 0, FATTR4_WORD2_LINUX_IMA };
+
 static void nfs4_bitmap_copy_adjust(__u32 *dst, const __u32 *src,
 		struct inode *inode)
 {
@@ -3575,7 +3580,7 @@ static void nfs4_close_context(struct nfs_open_context *ctx, int is_sync)
 
 #define FATTR4_WORD1_NFS40_MASK (2*FATTR4_WORD1_MOUNTED_ON_FILEID - 1UL)
 #define FATTR4_WORD2_NFS41_MASK (2*FATTR4_WORD2_SUPPATTR_EXCLCREAT - 1UL)
-#define FATTR4_WORD2_NFS42_MASK (2*FATTR4_WORD2_MODE_UMASK - 1UL)
+#define FATTR4_WORD2_NFS42_MASK (2*FATTR4_WORD2_LINUX_IMA - 1UL)
 
 static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *fhandle)
 {
@@ -3621,7 +3626,8 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 				NFS_CAP_MODE|NFS_CAP_NLINK|NFS_CAP_OWNER|
 				NFS_CAP_OWNER_GROUP|NFS_CAP_ATIME|
 				NFS_CAP_CTIME|NFS_CAP_MTIME|
-				NFS_CAP_SECURITY_LABEL);
+				NFS_CAP_SECURITY_LABEL|
+				NFS_CAP_IMA);
 		if (res.attr_bitmask[0] & FATTR4_WORD0_ACL &&
 				res.acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
 			server->caps |= NFS_CAP_ACLS;
@@ -3648,6 +3654,12 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 #ifdef CONFIG_NFS_V4_SECURITY_LABEL
 		if (res.attr_bitmask[2] & FATTR4_WORD2_SECURITY_LABEL)
 			server->caps |= NFS_CAP_SECURITY_LABEL;
+#ifdef CONFIG_IMA
+		if (res.attr_bitmask[2] & FATTR4_WORD2_LINUX_IMA) {
+			res.attr_bitmask[2] &= ~FATTR4_WORD2_LINUX_IMA;
+			server->caps |= NFS_CAP_IMA;
+		}
+#endif
 #endif
 		memcpy(server->attr_bitmask_nl, res.attr_bitmask,
 				sizeof(server->attr_bitmask));
@@ -5723,6 +5735,85 @@ static int nfs4_do_set_security_label(struct inode *inode,
 out:
 	return status;
 }
+
+#ifdef CONFIG_IMA
+
+static int _nfs4_get_security_ima(struct inode *inode, void *buf, size_t buflen)
+{
+	struct nfs_server *server = NFS_SERVER(inode);
+	struct nfs4_getima_arg arg = {
+		.fh = NFS_FH(inode),
+		.bitmask = nfs4_ima_bitmap,
+	};
+	struct nfs4_getima_res res = {
+		.data = buf,
+		.len = buflen,
+		.server = server,
+	};
+	struct rpc_message msg = {
+		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_GETIMA],
+		.rpc_argp = &arg,
+		.rpc_resp = &res,
+	};
+
+	return nfs4_call_sync(server->client, server, &msg, &arg.seq_args,
+			      &res.seq_res, 0);
+}
+
+static int nfs4_get_security_ima(struct inode *inode, void *buf, size_t buflen)
+{
+	struct nfs4_exception exception = {};
+	int err;
+
+	if (!nfs_server_capable(inode, NFS_CAP_IMA))
+		return -EOPNOTSUPP;
+	do {
+		err = _nfs4_get_security_ima(inode, buf, buflen);
+		err = nfs4_handle_exception(NFS_SERVER(inode), err, &exception);
+	} while (exception.retry);
+	return err;
+}
+
+static int _nfs4_set_security_ima(struct inode *inode, const void *buf,
+				  size_t buflen)
+{
+	struct nfs_server *server = NFS_SERVER(inode);
+	struct nfs4_setima_arg arg = {
+		.fh = NFS_FH(inode),
+		.data = buf,
+		.len = buflen,
+	};
+	struct nfs_setattrres res = {
+		.server = server,
+	};
+	struct rpc_message msg = {
+		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETIMA],
+		.rpc_argp = &arg,
+		.rpc_resp = &res,
+	};
+
+	nfs4_stateid_copy(&arg.stateid, &zero_stateid);
+	return nfs4_call_sync(server->client, server, &msg, &arg.seq_args,
+			      &res.seq_res, 1);
+}
+
+static int nfs4_set_security_ima(struct inode *inode, const void *buf,
+				 size_t buflen)
+{
+	struct nfs4_exception exception = {};
+	int err;
+
+	if (!nfs_server_capable(inode, NFS_CAP_IMA))
+		return -EOPNOTSUPP;
+	do {
+		err = _nfs4_set_security_ima(inode, buf, buflen);
+		err = nfs4_handle_exception(NFS_SERVER(inode), err, &exception);
+	} while (exception.retry);
+	return err;
+}
+
+#endif /* CONFIG_IMA */
+
 #endif	/* CONFIG_NFS_V4_SECURITY_LABEL */
 
 
@@ -7128,11 +7219,23 @@ static bool nfs4_xattr_list_nfs4_acl(struct dentry *dentry)
 
 #ifdef CONFIG_NFS_V4_SECURITY_LABEL
 
+#ifdef CONFIG_IMA
+/* XXX: security/ does not seem to provide this helper */
+static bool nfs4_security_isima(const char *name)
+{
+	return (strcmp(name, XATTR_IMA_SUFFIX) == 0);
+}
+#endif /* CONFIG_IMA */
+
 static int nfs4_xattr_set_security(const struct xattr_handler *handler,
 				   struct dentry *unused, struct inode *inode,
 				   const char *key, const void *buf,
 				   size_t buflen, int flags)
 {
+#ifdef CONFIG_IMA
+	if (nfs4_security_isima(key))
+		return nfs4_set_security_ima(inode, buf, buflen);
+#endif /* CONFIG_IMA */
 	if (security_ismaclabel(key))
 		return nfs4_set_security_label(inode, buf, buflen);
 	return -EOPNOTSUPP;
@@ -7142,6 +7245,10 @@ static int nfs4_xattr_get_security(const struct xattr_handler *handler,
 				   struct dentry *unused, struct inode *inode,
 				   const char *key, void *buf, size_t buflen)
 {
+#ifdef CONFIG_IMA
+	if (nfs4_security_isima(key))
+		return nfs4_get_security_ima(inode, buf, buflen);
+#endif /* CONFIG_IMA */
 	if (security_ismaclabel(key))
 		return nfs4_get_security_label(inode, buf, buflen);
 	return -EOPNOTSUPP;
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 2fc8f6f..01f9a70 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -434,6 +434,14 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
 #define decode_layoutget_maxsz	0
 #endif /* CONFIG_NFS_V4_1 */
 
+#define encode_getima_maxsz	(encode_getattr_maxsz)
+/* XXX: not quite right: we don't need getfattr_maxsz here */
+#define decode_getima_maxsz	(decode_getattr_maxsz + \
+				 1 + XDR_QUADLEN(NFS4_MAXIMALEN))
+#define encode_setima_maxsz	(encode_setattr_maxsz + \
+				 1 + XDR_QUADLEN(NFS4_MAXIMALEN))
+#define decode_setima_maxsz	(decode_setattr_maxsz)
+
 #define NFS4_enc_compound_sz	(1024)  /* XXX: large enough? */
 #define NFS4_dec_compound_sz	(1024)  /* XXX: large enough? */
 #define NFS4_enc_read_sz	(compound_encode_hdr_maxsz + \
@@ -901,6 +909,22 @@ static int decode_layoutget(struct xdr_stream *xdr, struct rpc_rqst *req,
 #define NFS4_dec_free_stateid_sz	(compound_decode_hdr_maxsz + \
 					 decode_sequence_maxsz + \
 					 decode_free_stateid_maxsz)
+#define NFS4_enc_getima_sz	(compound_encode_hdr_maxsz + \
+				 encode_sequence_maxsz + \
+				 encode_putfh_maxsz + \
+				 encode_getima_maxsz)
+#define NFS4_dec_getima_sz	(compound_decode_hdr_maxsz + \
+				 decode_sequence_maxsz + \
+				 decode_putfh_maxsz + \
+				 decode_getima_maxsz)
+#define NFS4_enc_setima_sz	(compound_encode_hdr_maxsz +\
+				 encode_sequence_maxsz + \
+				 encode_putfh_maxsz + \
+				 encode_setima_maxsz)
+#define NFS4_dec_setima_sz	(compound_decode_hdr_maxsz +\
+				 decode_sequence_maxsz + \
+				 decode_putfh_maxsz + \
+				 decode_setima_maxsz)
 
 const u32 nfs41_maxwrite_overhead = ((RPC_MAX_HEADER_WITH_AUTH +
 				      compound_encode_hdr_maxsz +
@@ -1280,6 +1304,15 @@ static void encode_fs_locations(struct xdr_stream *xdr, const u32* bitmask, stru
 			ARRAY_SIZE(nfs4_fs_locations_bitmap), hdr);
 }
 
+#if defined(CONFIG_NFS_V4_2)
+static void encode_getima(struct xdr_stream *xdr, const u32 *bitmask,
+			  struct compound_hdr *hdr)
+{
+	encode_getattr(xdr, nfs4_ima_bitmap, bitmask,
+		       ARRAY_SIZE(nfs4_ima_bitmap), hdr);
+}
+#endif /* CONFIG_NFS_V4_2 */
+
 static void encode_getfh(struct xdr_stream *xdr, struct compound_hdr *hdr)
 {
 	encode_op_hdr(xdr, OP_GETFH, decode_getfh_maxsz, hdr);
@@ -1698,6 +1731,19 @@ static void encode_setattr(struct xdr_stream *xdr, const struct nfs_setattrargs
 			server->attr_bitmask);
 }
 
+#if defined(CONFIG_NFS_V4_2)
+static void encode_setima(struct xdr_stream *xdr,
+			  const struct nfs4_setima_arg *arg,
+			  struct compound_hdr *hdr)
+{
+	encode_op_hdr(xdr, OP_SETATTR, decode_setattr_maxsz, hdr);
+	encode_nfs4_stateid(xdr, &arg->stateid);
+	xdr_encode_bitmap4(xdr, nfs4_ima_bitmap, ARRAY_SIZE(nfs4_ima_bitmap));
+	xdr_stream_encode_u32(xdr, sizeof(__be32) + xdr_align_size(arg->len));
+	xdr_stream_encode_opaque(xdr, arg->data, arg->len);
+}
+#endif	/* CONFIG_NFS_V4_2 */
+
 static void encode_setclientid(struct xdr_stream *xdr, const struct nfs4_setclientid *setclientid, struct compound_hdr *hdr)
 {
 	__be32 *p;
@@ -3144,6 +3190,44 @@ static void nfs4_xdr_enc_free_stateid(struct rpc_rqst *req,
 }
 #endif /* CONFIG_NFS_V4_1 */
 
+#if defined(CONFIG_NFS_V4_2)
+/*
+ * Encode GETATTR(IMA) request
+ */
+static void nfs4_xdr_enc_getima(struct rpc_rqst *req, struct xdr_stream *xdr,
+				const void *data)
+{
+	const struct nfs4_getima_arg *args = data;
+	struct compound_hdr hdr = {
+		.minorversion = nfs4_xdr_minorversion(&args->seq_args),
+	};
+
+	encode_compound_hdr(xdr, req, &hdr);
+	encode_sequence(xdr, &args->seq_args, &hdr);
+	encode_putfh(xdr, args->fh, &hdr);
+	encode_getima(xdr, args->bitmask, &hdr);
+	encode_nops(&hdr);
+}
+
+/*
+ * Encode an SETATTR(IMA) request
+ */
+static void nfs4_xdr_enc_setima(struct rpc_rqst *req, struct xdr_stream *xdr,
+				const void *data)
+{
+	const struct nfs4_setima_arg *args = data;
+	struct compound_hdr hdr = {
+		.minorversion = nfs4_xdr_minorversion(&args->seq_args),
+	};
+
+	encode_compound_hdr(xdr, req, &hdr);
+	encode_sequence(xdr, &args->seq_args, &hdr);
+	encode_putfh(xdr, args->fh, &hdr);
+	encode_setima(xdr, args, &hdr);
+	encode_nops(&hdr);
+}
+#endif /* CONFIG_NFS_V4_2 */
+
 static void print_overflow_msg(const char *func, const struct xdr_stream *xdr)
 {
 	dprintk("nfs: %s: prematurely hit end of receive buffer. "
@@ -4846,6 +4930,54 @@ static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
 	return decode_getfattr_generic(xdr, fattr, NULL, NULL, NULL, server);
 }
 
+#if defined(CONFIG_NFS_V4_2)
+static int decode_getima(struct xdr_stream *xdr, void *data, u32 len,
+			 const struct nfs_server *server)
+{
+	u32 attrlen, bitmap[3] = { 0 };
+	unsigned int savep;
+	int status;
+	u32 size;
+
+	status = decode_op_hdr(xdr, OP_GETATTR);
+	if (status < 0)
+		return status;
+	status = decode_attr_bitmap(xdr, bitmap);
+	if (status < 0)
+		return status;
+	status = decode_attr_length(xdr, &attrlen, &savep);
+	if (status < 0)
+		return status;
+
+	if (likely(bitmap[2] & FATTR4_WORD2_LINUX_IMA)) {
+		__be32 *p;
+
+		p = xdr_inline_decode(xdr, 4);
+		if (unlikely(!p))
+			return -EIO;
+		size = be32_to_cpup(p++);
+		p = xdr_inline_decode(xdr, size);
+		if (unlikely(!p))
+			return -EIO;
+
+		if (size > NFS4_MAXIMALEN)
+			return -E2BIG;
+		/* @len == 0 means "just return the size" */
+		if (len > 0) {
+			if (size > len)
+				return -ERANGE;
+			memcpy(data, p, len);
+		}
+	} else
+		return -ENODATA;
+
+	status = verify_attr_len(xdr, savep, attrlen);
+	if (status < 0)
+		return status;
+	return size;
+}
+#endif /* CONFIG_NFS_V4_2 */
+
 /*
  * Decode potentially multiple layout types.
  */
@@ -7547,6 +7679,58 @@ static int nfs4_xdr_dec_free_stateid(struct rpc_rqst *rqstp,
 }
 #endif /* CONFIG_NFS_V4_1 */
 
+#if defined(CONFIG_NFS_V4_2)
+
+/*
+ * Decode GETATTR(IMA) response
+ */
+static int nfs4_xdr_dec_getima(struct rpc_rqst *rqstp, struct xdr_stream *xdr,
+			       void *data)
+{
+	struct nfs4_getima_res *res = data;
+	struct compound_hdr hdr;
+	int status;
+
+	status = decode_compound_hdr(xdr, &hdr);
+	if (status)
+		goto out;
+	status = decode_sequence(xdr, &res->seq_res, rqstp);
+	if (status)
+		goto out;
+	status = decode_putfh(xdr);
+	if (status)
+		goto out;
+	status = decode_getima(xdr, res->data, res->len, res->server);
+out:
+	return status;
+}
+
+/*
+ * Decode SETATTR(IMA) response
+ */
+static int nfs4_xdr_dec_setima(struct rpc_rqst *rqstp, struct xdr_stream *xdr,
+			       void *data)
+{
+	struct nfs_setattrres *res = data;
+	struct compound_hdr hdr;
+	int status;
+
+	status = decode_compound_hdr(xdr, &hdr);
+	if (status)
+		goto out;
+	status = decode_sequence(xdr, &res->seq_res, rqstp);
+	if (status)
+		goto out;
+	status = decode_putfh(xdr);
+	if (status)
+		goto out;
+	status = decode_setattr(xdr);
+out:
+	return status;
+}
+
+#endif /* CONFIG_NFS_V4_2 */
+
 /**
  * nfs4_decode_dirent - Decode a single NFSv4 directory entry stored in
  *                      the local page cache.
@@ -7791,6 +7975,8 @@ int nfs4_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
 	PROC42(COPY,		enc_copy,		dec_copy),
 	PROC42(OFFLOAD_CANCEL,	enc_offload_cancel,	dec_offload_cancel),
 	PROC(LOOKUPP,		enc_lookupp,		dec_lookupp),
+	PROC42(GETIMA,		enc_getima,		dec_getima),
+	PROC42(SETIMA,		enc_setima,		dec_setima),
 };
 
 static unsigned int nfs_version4_counts[ARRAY_SIZE(nfs4_procedures)];
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index ca3adb1..4d5c771 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -540,6 +540,9 @@ enum {
 	NFSPROC4_CLNT_OFFLOAD_CANCEL,
 
 	NFSPROC4_CLNT_LOOKUPP,
+
+	NFSPROC4_CLNT_GETIMA,
+	NFSPROC4_CLNT_SETIMA,
 };
 
 /* nfs41 types */
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index 6aa8cc8..b92f954 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -261,5 +261,6 @@ struct nfs_server {
 #define NFS_CAP_CLONE		(1U << 23)
 #define NFS_CAP_COPY		(1U << 24)
 #define NFS_CAP_OFFLOAD_CANCEL	(1U << 25)
+#define NFS_CAP_IMA		(1U << 26)
 
 #endif
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 441a93e..4ee30f0 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1010,6 +1010,27 @@ struct nfs4_getattr_res {
 	struct nfs4_label		*label;
 };
 
+struct nfs4_getima_arg {
+	struct nfs4_sequence_args	seq_args;
+	const struct nfs_fh		*fh;
+	const u32			*bitmask;
+};
+
+struct nfs4_getima_res {
+	struct nfs4_sequence_res	seq_res;
+	const struct nfs_server		*server;
+	void				*data;
+	u32				len;
+};
+
+struct nfs4_setima_arg {
+	struct nfs4_sequence_args	seq_args;
+	struct nfs_fh			*fh;
+	nfs4_stateid                    stateid;
+	const void			*data;
+	u32				len;
+};
+
 struct nfs4_link_arg {
 	struct nfs4_sequence_args 	seq_args;
 	const struct nfs_fh *		fh;

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[J. Bruce Fields]

On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
> The NFS server needs to allow NFS clients to perform their own
> attestation and measurement.

Can we really remove this call?

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/vfs.c |    6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 3c00072..524c6e5 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
>  		goto out_nfserr;
>  	}
>  
> -	host_err = ima_file_check(file, may_flags);
> -	if (host_err) {
> -		fput(file);
> -		goto out_nfserr;
> -	}
> -
>  	if (may_flags & NFSD_MAY_64BIT_COOKIE)
>  		file->f_mode |= FMODE_64BITHASH;
>  	else

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Chuck Lever]

> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
> 
> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
>> The NFS server needs to allow NFS clients to perform their own
>> attestation and measurement.
> 
> Can we really remove this call?

Why wouldn't we be able to?


> --b.
> 
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfsd/vfs.c |    6 ------
>> 1 file changed, 6 deletions(-)
>> 
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 3c00072..524c6e5 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
>> 		goto out_nfserr;
>> 	}
>> 
>> -	host_err = ima_file_check(file, may_flags);
>> -	if (host_err) {
>> -		fput(file);
>> -		goto out_nfserr;
>> -	}
>> -
>> 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
>> 		file->f_mode |= FMODE_64BITHASH;
>> 	else

--
Chuck Lever

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Bruce Fields]

On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
> 
> 
> > On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
> > 
> > On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
> >> The NFS server needs to allow NFS clients to perform their own
> >> attestation and measurement.
> > 
> > Can we really remove this call?
> 
> Why wouldn't we be able to?

I don't know the first thing about IMA, but surely it's there for some
reason--is it really OK just to skip this on opens by nfsd?

--b.

> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> fs/nfsd/vfs.c |    6 ------
> >> 1 file changed, 6 deletions(-)
> >> 
> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >> index 3c00072..524c6e5 100644
> >> --- a/fs/nfsd/vfs.c
> >> +++ b/fs/nfsd/vfs.c
> >> @@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
> >> 		goto out_nfserr;
> >> 	}
> >> 
> >> -	host_err = ima_file_check(file, may_flags);
> >> -	if (host_err) {
> >> -		fput(file);
> >> -		goto out_nfserr;
> >> -	}
> >> -
> >> 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
> >> 		file->f_mode |= FMODE_64BITHASH;
> >> 	else
> 
> --
> Chuck Lever
> 
> 

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Chuck Lever]

> On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
>> 
>> 
>>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
>>> 
>>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
>>>> The NFS server needs to allow NFS clients to perform their own
>>>> attestation and measurement.
>>> 
>>> Can we really remove this call?
>> 
>> Why wouldn't we be able to?
> 
> I don't know the first thing about IMA, but surely it's there for some
> reason--

It was originally added because the number of opens and closes of @file
were counted, and not having that call was triggering a warning. Since
commit 8eb988c70e770 ("fix ima breakage") the counters are maintained
separately.


> is it really OK just to skip this on opens by nfsd?

That's why I split this out into a separate patch. I'm hoping to get
some commentary from the linux-integrity community.


> --b.
> 
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>> fs/nfsd/vfs.c |    6 ------
>>>> 1 file changed, 6 deletions(-)
>>>> 
>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>> index 3c00072..524c6e5 100644
>>>> --- a/fs/nfsd/vfs.c
>>>> +++ b/fs/nfsd/vfs.c
>>>> @@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
>>>> 		goto out_nfserr;
>>>> 	}
>>>> 
>>>> -	host_err = ima_file_check(file, may_flags);
>>>> -	if (host_err) {
>>>> -		fput(file);
>>>> -		goto out_nfserr;
>>>> -	}
>>>> -
>>>> 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
>>>> 		file->f_mode |= FMODE_64BITHASH;
>>>> 	else
>> 
>> --
>> Chuck Lever
>> 
>> 

--
Chuck Lever

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Mimi Zohar]

On Fri, 2019-03-08 at 16:29 -0500, Chuck Lever wrote:

Thanks Serge for bringing this thread to my attention.  Sorry for the
delay in responding ...

> > On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
> >> 
> >> 
> >>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
> >>> 
> >>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
> >>>> The NFS server needs to allow NFS clients to perform their own
> >>>> attestation and measurement.

Measurement and attestation is only one aspect.  The other aspect is
verifying the integrity of files.  Shouldn't the NFS server verify the
integrity of a file before allowing it to be served (eg. malware)?

> >>> 
> >>> Can we really remove this call?
> >> 
> >> Why wouldn't we be able to?
> > 
> > I don't know the first thing about IMA, but surely it's there for some
> > reason--
> 
> It was originally added because the number of opens and closes of @file
> were counted, and not having that call was triggering a warning. Since
> commit 8eb988c70e770 ("fix ima breakage") the counters are maintained
> separately.

If that was the only reason, then the call itself would have been
removed with the counter code.

Mimi

> 
> 
> > is it really OK just to skip this on opens by nfsd?
> 
> That's why I split this out into a separate patch. I'm hoping to get
> some commentary from the linux-integrity community.
> 
> 
> > --b.
> > 
> >>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>> ---
> >>>> fs/nfsd/vfs.c |    6 ------
> >>>> 1 file changed, 6 deletions(-)
> >>>> 
> >>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >>>> index 3c00072..524c6e5 100644
> >>>> --- a/fs/nfsd/vfs.c
> >>>> +++ b/fs/nfsd/vfs.c
> >>>> @@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
> >>>> 		goto out_nfserr;
> >>>> 	}
> >>>> 
> >>>> -	host_err = ima_file_check(file, may_flags);
> >>>> -	if (host_err) {
> >>>> -		fput(file);
> >>>> -		goto out_nfserr;
> >>>> -	}
> >>>> -
> >>>> 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
> >>>> 		file->f_mode |= FMODE_64BITHASH;
> >>>> 	else
> >> 
> >> --
> >> Chuck Lever
> >> 
> >> 
> 
> --
> Chuck Lever
> 
> 
> 

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Chuck Lever]

> On Mar 19, 2019, at 3:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2019-03-08 at 16:29 -0500, Chuck Lever wrote:
> 
> Thanks Serge for bringing this thread to my attention.  Sorry for the
> delay in responding ...
> 
>>> On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>> 
>>> On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
>>>> 
>>>> 
>>>>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
>>>>> 
>>>>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
>>>>>> The NFS server needs to allow NFS clients to perform their own
>>>>>> attestation and measurement.
> 
> Measurement and attestation is only one aspect.  The other aspect is
> verifying the integrity of files.  Shouldn't the NFS server verify the
> integrity of a file before allowing it to be served (eg. malware)?

Hi Mimi, thanks for the review.

Architecturally, the server is not using the file's data, it is
merely part of the filesystem that stores it. But that said, there
are several concrete reasons why I feel an NFS server should not be
involved in measurement/attestation, but only with storing file
content and IMA metadata.

1. The broadest attack surface for a remote filesystem is modification
of data in flight. Attestation of the file on the server is not going
to defend against that attack, only attestation on the client will do
that. Is there a good reason to pay the cost of double attestation?

2. It is possible (perhaps even likely) that the NFS server and a
client of that server will have different IMA policies and even
different file signing authorities.

A third, perhaps related, reason is that NFS can run on non-Linux NFS
servers which would not have any attestation at all. An NFS client
should not have to rely on the server for attestation, but should
trust only its own measurement of each file, which would be done as
late as possible before use.

Lastly, the NFS protocol does not enable an NFS client to tell a
server how the file is to be used. For example, the server's policy
might block execution of an unverifiable file, but the server won't
have any way of knowing how the client is going to use that file.
The client might be opening the file to copy it or update its IMA
metadata.

Speaking of protocol, there's no special error code that reports an
integrity verification failure. The client just sees that the UID
does not have access to the file. There's no way the user or client
can do anything to clear this condition via NFS without IMA support.

If these reasons make sense, should I add them to the patch description?


>>>>> Can we really remove this call?
>>>> 
>>>> Why wouldn't we be able to?
>>> 
>>> I don't know the first thing about IMA, but surely it's there for some
>>> reason--
>> 
>> It was originally added because the number of opens and closes of @file
>> were counted, and not having that call was triggering a warning. Since
>> commit 8eb988c70e770 ("fix ima breakage") the counters are maintained
>> separately.
> 
> If that was the only reason, then the call itself would have been
> removed with the counter code.
> 
> Mimi
> 
>> 
>> 
>>> is it really OK just to skip this on opens by nfsd?
>> 
>> That's why I split this out into a separate patch. I'm hoping to get
>> some commentary from the linux-integrity community.
>> 
>> 
>>> --b.
>>> 
>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>> ---
>>>>>> fs/nfsd/vfs.c |    6 ------
>>>>>> 1 file changed, 6 deletions(-)
>>>>>> 
>>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>>> index 3c00072..524c6e5 100644
>>>>>> --- a/fs/nfsd/vfs.c
>>>>>> +++ b/fs/nfsd/vfs.c
>>>>>> @@ -802,12 +802,6 @@ static int nfsd_open_break_lease(struct inode *inode, int access)
>>>>>> 		goto out_nfserr;
>>>>>> 	}
>>>>>> 
>>>>>> -	host_err = ima_file_check(file, may_flags);
>>>>>> -	if (host_err) {
>>>>>> -		fput(file);
>>>>>> -		goto out_nfserr;
>>>>>> -	}
>>>>>> -
>>>>>> 	if (may_flags & NFSD_MAY_64BIT_COOKIE)
>>>>>> 		file->f_mode |= FMODE_64BITHASH;
>>>>>> 	else
>>>> 
>>>> --
>>>> Chuck Lever
>>>> 
>>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> 

--
Chuck Lever

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Mimi Zohar]

On Wed, 2019-03-20 at 08:40 -0500, Chuck Lever wrote:
> 
> > On Mar 19, 2019, at 3:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Fri, 2019-03-08 at 16:29 -0500, Chuck Lever wrote:
> > 
> > Thanks Serge for bringing this thread to my attention.  Sorry for the
> > delay in responding ...
> > 
> >>> On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
> >>> 
> >>> On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
> >>>> 
> >>>> 
> >>>>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
> >>>>> 
> >>>>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
> >>>>>> The NFS server needs to allow NFS clients to perform their own
> >>>>>> attestation and measurement.
> > 
> > Measurement and attestation is only one aspect.  The other aspect is
> > verifying the integrity of files.  Shouldn't the NFS server verify the
> > integrity of a file before allowing it to be served (eg. malware)?
> 
> Hi Mimi, thanks for the review.
> 
> Architecturally, the server is not using the file's data, it is
> merely part of the filesystem that stores it. But that said, there
> are several concrete reasons why I feel an NFS server should not be
> involved in measurement/attestation, but only with storing file
> content and IMA metadata.

"Remote attestation" is the process of verifying the measurement list
against the TPM PCRs, based on a TPM quote.  I think you meant
"measurement/appraisal".

> 
> 1. The broadest attack surface for a remote filesystem is modification
> of data in flight. Attestation of the file on the server is not going
> to defend against that attack, only attestation on the client will do
> that. Is there a good reason to pay the cost of double attestation?

Doesn't the server have a responsibility to provide files that have
not been unintentionally or maliciously altered?

> 2. It is possible (perhaps even likely) that the NFS server and a
> client of that server will have different IMA policies and even
> different file signing authorities.

That doesn't negate the due diligence on the server's part of
preventing the spread of malware.
> 
> A third, perhaps related, reason is that NFS can run on non-Linux NFS
> servers which would not have any attestation at all. An NFS client
> should not have to rely on the server for attestation, but should
> trust only its own measurement of each file, which would be done as
> late as possible before use.

The ima_file_check() hook can also audit the file, providing
additional forensic information (eg. the file hash).

Mimi

> 
> Lastly, the NFS protocol does not enable an NFS client to tell a
> server how the file is to be used. For example, the server's policy
> might block execution of an unverifiable file, but the server won't
> have any way of knowing how the client is going to use that file.
> The client might be opening the file to copy it or update its IMA
> metadata.
> 
> Speaking of protocol, there's no special error code that reports an
> integrity verification failure. The client just sees that the UID
> does not have access to the file. There's no way the user or client
> can do anything to clear this condition via NFS without IMA support.
> 
> If these reasons make sense, should I add them to the patch description?
> 

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Chuck Lever]

> On Mar 21, 2019, at 6:44 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Wed, 2019-03-20 at 08:40 -0500, Chuck Lever wrote:
>> 
>>> On Mar 19, 2019, at 3:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Fri, 2019-03-08 at 16:29 -0500, Chuck Lever wrote:
>>> 
>>> Thanks Serge for bringing this thread to my attention.  Sorry for the
>>> delay in responding ...
>>> 
>>>>> On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>>>> 
>>>>> On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
>>>>>>> 
>>>>>>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
>>>>>>>> The NFS server needs to allow NFS clients to perform their own
>>>>>>>> attestation and measurement.
>>> 
>>> Measurement and attestation is only one aspect.  The other aspect is
>>> verifying the integrity of files.  Shouldn't the NFS server verify the
>>> integrity of a file before allowing it to be served (eg. malware)?
>> 
>> Hi Mimi, thanks for the review.
>> 
>> Architecturally, the server is not using the file's data, it is
>> merely part of the filesystem that stores it. But that said, there
>> are several concrete reasons why I feel an NFS server should not be
>> involved in measurement/attestation, but only with storing file
>> content and IMA metadata.
> 
> "Remote attestation" is the process of verifying the measurement list
> against the TPM PCRs, based on a TPM quote.  I think you meant
> "measurement/appraisal".
> 
>> 
>> 1. The broadest attack surface for a remote filesystem is modification
>> of data in flight. Attestation of the file on the server is not going
>> to defend against that attack, only attestation on the client will do
>> that. Is there a good reason to pay the cost of double attestation?
> 
> Doesn't the server have a responsibility to provide files that have
> not been unintentionally or maliciously altered?

It's a design goal of any filesystem to present unaltered file data
to applications. But the responsibility is end-to-end. Adding extra
checks in the middle introduce a cost. Measuring on the client is
sufficient, and it is equivalent to what local filesystems do (and,
it allows each client to apply its own security policy).

I'm going to claim here without proof that there is little value in
using IMA on an NFS server that serves NFS clients that are not
IMA-aware. :-)


>> 2. It is possible (perhaps even likely) that the NFS server and a
>> client of that server will have different IMA policies and even
>> different file signing authorities.
> 
> That doesn't negate the due diligence on the server's part of
> preventing the spread of malware.

Commercial NFS servers (like NetApp filers) perform malware and
integrity checking via a scrubbing agent rather than checking in a
hot path. Filesystems are not only responsible for leaving data
unchanged, they also have performance requirements.


>> A third, perhaps related, reason is that NFS can run on non-Linux NFS
>> servers which would not have any attestation at all. An NFS client
>> should not have to rely on the server for attestation, but should
>> trust only its own measurement of each file, which would be done as
>> late as possible before use.
> 
> The ima_file_check() hook can also audit the file, providing
> additional forensic information (eg. the file hash).

IIUC, you are talking about troubleshooting, which should be
rare. That can be done with tools on the server if needed, but
IMO can be avoided in performance-sensitive paths.


> Mimi
> 
>> 
>> Lastly, the NFS protocol does not enable an NFS client to tell a
>> server how the file is to be used. For example, the server's policy
>> might block execution of an unverifiable file, but the server won't
>> have any way of knowing how the client is going to use that file.
>> The client might be opening the file to copy it or update its IMA
>> metadata.
>> 
>> Speaking of protocol, there's no special error code that reports an
>> integrity verification failure. The client just sees that the UID
>> does not have access to the file. There's no way the user or client
>> can do anything to clear this condition via NFS without IMA support.
>> 
>> If these reasons make sense, should I add them to the patch description?

--
Chuck Lever

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Mimi Zohar]

On Thu, 2019-03-21 at 09:04 -0500, Chuck Lever wrote:
> 
> > On Mar 21, 2019, at 6:44 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Wed, 2019-03-20 at 08:40 -0500, Chuck Lever wrote:
> >>> On Mar 19, 2019, at 3:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> On Fri, 2019-03-08 at 16:29 -0500, Chuck Lever wrote:
> >>> Thanks Serge for bringing this thread to my attention.  Sorry for the
> >>>>> On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
> >>>>> On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
> >>>>>>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
> >>>>>>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
> >>>>>>>> The NFS server needs to allow NFS clients to perform their own
> >>>>>>>> attestation and measurement.
> >>> 
> >>> Measurement and attestation is only one aspect.  The other aspect is
> >>> verifying the integrity of files.  Shouldn't the NFS server verify the
> >>> integrity of a file before allowing it to be served (eg. malware)?
> >> 
> >> Hi Mimi, thanks for the review.
> >> 
> >> Architecturally, the server is not using the file's data, it is
> >> merely part of the filesystem that stores it. But that said, there
> >> are several concrete reasons why I feel an NFS server should not be
> >> involved in measurement/attestation, but only with storing file
> >> content and IMA metadata.
> > 
> > "Remote attestation" is the process of verifying the measurement list
> > against the TPM PCRs, based on a TPM quote.  I think you meant
> > "measurement/appraisal".
> > 
> >> 
> >> 1. The broadest attack surface for a remote filesystem is modification
> >> of data in flight. Attestation of the file on the server is not going
> >> to defend against that attack, only attestation on the client will do
> >> that. Is there a good reason to pay the cost of double attestation?
> > 
> > Doesn't the server have a responsibility to provide files that have
> > not been unintentionally or maliciously altered?
> 
> It's a design goal of any filesystem to present unaltered file data
> to applications. But the responsibility is end-to-end. Adding extra
> checks in the middle introduce a cost. 

Files are measured/appraised/audited based on the IMA policy.  Have
you measured the performance cost of measuring and appraising the
files being served?  Unless a policy has been supplied, the
performance impact, if any, would be limited to walking the IMA policy
rules.

> Measuring on the client is
> sufficient, and it is equivalent to what local filesystems do (and,
> it allows each client to apply its own security policy).

I'm not arguing with you about an end-to-end file integrity solution.
 That is the goal, but one that assumes this proposed work, based on
fs-verity signatures.

> I'm going to claim here without proof that there is little value in
> using IMA on an NFS server that serves NFS clients that are not
> IMA-aware. :-)

For systems that don't or haven't implemented the proposed end-to-end
file integrity solution, verifying the file integrity on the server is
all the more important.

> 
> >> 2. It is possible (perhaps even likely) that the NFS server and a
> >> client of that server will have different IMA policies and even
> >> different file signing authorities.
> > 
> > That doesn't negate the due diligence on the server's part of
> > preventing the spread of malware.
> 
> Commercial NFS servers (like NetApp filers) perform malware and
> integrity checking via a scrubbing agent rather than checking in a
> hot path. Filesystems are not only responsible for leaving data
> unchanged, they also have performance requirements.

Any userspace application leaves a window of opportunity between the
time the file has been created/modified and the time that the
application verifies it.  This is one of the main reason for IMA being
 in the kernel.

> 
> >> A third, perhaps related, reason is that NFS can run on non-Linux NFS
> >> servers which would not have any attestation at all. An NFS client
> >> should not have to rely on the server for attestation, but should
> >> trust only its own measurement of each file, which would be done as
> >> late as possible before use.
> > 
> > The ima_file_check() hook can also audit the file, providing
> > additional forensic information (eg. the file hash).
> 
> IIUC, you are talking about troubleshooting, which should be
> rare. That can be done with tools on the server if needed, but
> IMO can be avoided in performance-sensitive paths.

No, this isn't about "troubleshooting", but about auditing the files
served and using the file hashes for forensic investigations.[1][2]

Mimi

[1] Commit e7c568e0fd0c ("ima: audit log hashes")
[2] https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.html

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Chuck Lever]

> On Mar 22, 2019, at 6:55 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Thu, 2019-03-21 at 09:04 -0500, Chuck Lever wrote:
>> 
>>> On Mar 21, 2019, at 6:44 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> On Wed, 2019-03-20 at 08:40 -0500, Chuck Lever wrote:
>>>>> On Mar 19, 2019, at 3:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> On Fri, 2019-03-08 at 16:29 -0500, Chuck Lever wrote:
>>>>> Thanks Serge for bringing this thread to my attention.  Sorry for the
>>>>>>> On Mar 8, 2019, at 4:23 PM, Bruce Fields <bfields@fieldses.org> wrote:
>>>>>>> On Fri, Mar 08, 2019 at 04:11:06PM -0500, Chuck Lever wrote:
>>>>>>>>> On Mar 8, 2019, at 4:10 PM, bfields@fieldses.org wrote:
>>>>>>>>> On Thu, Mar 07, 2019 at 10:28:54AM -0500, Chuck Lever wrote:
>>>>>>>>>> The NFS server needs to allow NFS clients to perform their own
>>>>>>>>>> attestation and measurement.
>>>>> 
>>>>> Measurement and attestation is only one aspect.  The other aspect is
>>>>> verifying the integrity of files.  Shouldn't the NFS server verify the
>>>>> integrity of a file before allowing it to be served (eg. malware)?
>>>> 
>>>> Hi Mimi, thanks for the review.
>>>> 
>>>> Architecturally, the server is not using the file's data, it is
>>>> merely part of the filesystem that stores it. But that said, there
>>>> are several concrete reasons why I feel an NFS server should not be
>>>> involved in measurement/attestation, but only with storing file
>>>> content and IMA metadata.
>>> 
>>> "Remote attestation" is the process of verifying the measurement list
>>> against the TPM PCRs, based on a TPM quote.  I think you meant
>>> "measurement/appraisal".
>>> 
>>>> 
>>>> 1. The broadest attack surface for a remote filesystem is modification
>>>> of data in flight. Attestation of the file on the server is not going
>>>> to defend against that attack, only attestation on the client will do
>>>> that. Is there a good reason to pay the cost of double attestation?
>>> 
>>> Doesn't the server have a responsibility to provide files that have
>>> not been unintentionally or maliciously altered?
>> 
>> It's a design goal of any filesystem to present unaltered file data
>> to applications. But the responsibility is end-to-end. Adding extra
>> checks in the middle introduce a cost. 
> 
> Files are measured/appraised/audited based on the IMA policy.  Have
> you measured the performance cost of measuring and appraising the
> files being served?  Unless a policy has been supplied, the
> performance impact, if any, would be limited to walking the IMA policy
> rules.

I have not measured the performance cost, but I'm simply observing
that it could be substantial in some cases.

nfsd_open is called in the NFSv3 READ and WRITE paths, in addition to
whenever a file is locked (NFSv3) or opened (NFSv4). The impact is
small when there is no IMA policy, but could be substantial when there
is an IMA policy in effect.

Note that NFSv3 does not have an OPEN operation. I'm not sure what
would be an adequate substitute location for an IMA hash audit for
NFSv3 file accesses.

Measuring on the client gives us the most effective integrity coverage
and an implementation that is closest in behavior to local filesystem
semantics. I'm just saying I'd rather not measure twice, because it
doesn't provide a great benefit (more below) and can be costly. I agree
that we should figure out a way to assess its performance impact.


>> Measuring on the client is
>> sufficient, and it is equivalent to what local filesystems do (and,
>> it allows each client to apply its own security policy).
> 
> I'm not arguing with you about an end-to-end file integrity solution.
>  That is the goal, but one that assumes this proposed work, based on
> fs-verity signatures.
> 
>> I'm going to claim here without proof that there is little value in
>> using IMA on an NFS server that serves NFS clients that are not
>> IMA-aware. :-)
> 
> For systems that don't or haven't implemented the proposed end-to-end
> file integrity solution, verifying the file integrity on the server is
> all the more important.

As I said before, the largest integrity exposure is modification of
file data as it is transported between server and client -- _after_ the
server has measured it. IMHO that makes measuring on the server a bit
of security theatre.

In addition to that, ima_file_check returns -EACCES if it fails (and
the IMA policy is in enforcing mode). That is a confusing situation
for NFS, since that looks like the user does not have permission to
access the file (ie, that it can be corrected by the user by changing
an ACL or mode bits).

Maybe NFS should have new error codes that can convey "blocked by
administrative policy" or "failed integrity check". In the meantime,
though, existing NFS systems without any IMA support do not have these
codes.


>>>> 2. It is possible (perhaps even likely) that the NFS server and a
>>>> client of that server will have different IMA policies and even
>>>> different file signing authorities.
>>> 
>>> That doesn't negate the due diligence on the server's part of
>>> preventing the spread of malware.
>> 
>> Commercial NFS servers (like NetApp filers) perform malware and
>> integrity checking via a scrubbing agent rather than checking in a
>> hot path. Filesystems are not only responsible for leaving data
>> unchanged, they also have performance requirements.
> 
> Any userspace application leaves a window of opportunity between the
> time the file has been created/modified and the time that the
> application verifies it.  This is one of the main reason for IMA being
> in the kernel.
> 
>> 
>>>> A third, perhaps related, reason is that NFS can run on non-Linux NFS
>>>> servers which would not have any attestation at all. An NFS client
>>>> should not have to rely on the server for attestation, but should
>>>> trust only its own measurement of each file, which would be done as
>>>> late as possible before use.
>>> 
>>> The ima_file_check() hook can also audit the file, providing
>>> additional forensic information (eg. the file hash).
>> 
>> IIUC, you are talking about troubleshooting, which should be
>> rare. That can be done with tools on the server if needed, but
>> IMO can be avoided in performance-sensitive paths.
> 
> No, this isn't about "troubleshooting", but about auditing the files
> served and using the file hashes for forensic investigations.[1][2]
> 
> Mimi
> 
> [1] Commit e7c568e0fd0c ("ima: audit log hashes")
> [2] https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.html

Auditing can be done by keeping the ima_file_check call site but
ignoring its return code, for example.

In any event, removing the ima_file_check call is not required for
the prototype to be functional. I can drop this patch for now, but
I encourage examination of how the NFS server measures and audits
files when an actual IMA policy is in effect.


--
Chuck Lever

Re: [PATCH v2 3/5] NFSD: Remove ima_file_check call

[Mimi Zohar]

On Mon, 2019-03-25 at 10:24 -0400, Chuck Lever wrote:

> Auditing can be done by keeping the ima_file_check call site but
> ignoring its return code, for example.

Neither the "measure" or "audit" rules control the return code.  Only
an IMA "appraise" rule verifies a file's integrity, which could fail,
resulting in an error return code.

Different systems might have different requirements.  Having the IMA
hook here, allows IMA custom policies to be defined, based on the
specific system requirements.

In terms of performance, IMA calculates the file hash once, which can
then be used for measuring, appraising, and auditing.  Unless the file
changes, calculating the file hash is only done once.

> In any event, removing the ima_file_check call is not required for
> the prototype to be functional. I can drop this patch for now, but
> I encourage examination of how the NFS server measures and audits
> files when an actual IMA policy is in effect.

Thank you!

Mimi