This patch set is the first submission to fs-devel and lkml for the purpose of
code review. To test the patch set you need patches to nfs-utils as well. Since
this is just a code review I haven't posted the patch to nfs-utils however if
you want to test the code feel free to e-mail me and I will send you the
necessary patch.
Out of all of the functionality we have prototyped I have narrowed it down to
these items which I believe is the solid base for initial kernel inclusion.
These patches provide the mechanism to allow the server to provide security
labels to the client and a method for the client to change labels on the server.
The next revision of this patch set will allow for the client's subject
(process) label to be transmitted with the access requests so the server can
also make access decisions against the acting local policy. This part of the
patch set will be made substantially cleaner by the credentials patches
proposed by David Howells.
Known Issues:
Eventually stronger notification of security label changes will be added. For
now this is accomplished by using NFS's normal cache invalidation (timeout).
When acting as root on a root_squashed export changing the label on a file
manages to set the label locally in the NFS inode but doesn't set it on the
exported file system. In this case the fault is the server is returning OK for
the setattr option instead of EPERM. This will be fixed in the next version.
The existing nfs4 xattr handlers do not accept xattr calls to the security
namespace. This patch extends these handlers to accept xattrs from the security
namespace in addition to the default nfsv4 acl namespace.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/nfs/nfs4proc.c | 54 +++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index a1a4051..d7193df 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3910,10 +3910,13 @@ int nfs4_setxattr(struct dentry *dentry, const char *key, const void *buf,
{
struct inode *inode = dentry->d_inode;
- if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
- return -EOPNOTSUPP;
-
- return nfs4_proc_set_acl(inode, buf, buflen);
+ if (strcmp(key, XATTR_NAME_NFSV4_ACL) == 0)
+ return nfs4_proc_set_acl(inode, buf, buflen);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (strcmp(key, security_maclabel_getname()) == 0)
+ return nfs4_set_security_label(dentry, buf, buflen);
+#endif
+ return -EOPNOTSUPP;
}
/* The getxattr man page suggests returning -ENODATA for unknown attributes,
@@ -3925,22 +3928,49 @@ ssize_t nfs4_getxattr(struct dentry *dentry, const char *key, void *buf,
{
struct inode *inode = dentry->d_inode;
- if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
- return -EOPNOTSUPP;
-
- return nfs4_proc_get_acl(inode, buf, buflen);
+ if (strcmp(key, XATTR_NAME_NFSV4_ACL) == 0)
+ return nfs4_proc_get_acl(inode, buf, buflen);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (strcmp(key, security_maclabel_getname()) == 0)
+ return nfs4_get_security_label(inode, buf, buflen);
+#endif
+ return -EOPNOTSUPP;
}
ssize_t nfs4_listxattr(struct dentry *dentry, char *buf, size_t buflen)
{
- size_t len = strlen(XATTR_NAME_NFSV4_ACL) + 1;
+ size_t len = 0, l;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ const char *key = security_maclabel_getname();
+#endif
+ char *p;
- if (!nfs4_server_supports_acls(NFS_SERVER(dentry->d_inode)))
+ if (nfs4_server_supports_acls(NFS_SERVER(dentry->d_inode)))
+ len += strlen(XATTR_NAME_NFSV4_ACL) + 1;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dentry->d_inode, NFS_CAP_SECURITY_LABEL))
+ len += strlen(key) + 1;
+#endif
+ if (!len)
return 0;
if (buf && buflen < len)
return -ERANGE;
- if (buf)
- memcpy(buf, XATTR_NAME_NFSV4_ACL, len);
+ if (!buf)
+ return len;
+
+ p = buf;
+ if (nfs4_server_supports_acls(NFS_SERVER(dentry->d_inode))) {
+ l = strlen(XATTR_NAME_NFSV4_ACL) + 1;
+ memcpy(p, XATTR_NAME_NFSV4_ACL, l);
+ p += l;
+ }
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dentry->d_inode, NFS_CAP_SECURITY_LABEL)) {
+ l = strlen(key) + 1;
+ memcpy(p, key, l);
+ p += l;
+ }
+#endif
return len;
}
--
1.5.3.8
There currently doesn't exist a labeling type that is adequate for use with
labeled nfs. Since NFS doesn't really support xattrs we can't use the use xattr
labeling behavior. For this we developed a new labeling type. The native
labeling type is used solely by NFS to ensure nfs inodes are labeled at runtime
by the NFS code instead of relying on the SELinux security server on the client
end.
Signed-off-by: David P. Quigley <[email protected]>
---
security/selinux/hooks.c | 20 ++++++++++++--------
security/selinux/include/security.h | 2 ++
security/selinux/ss/policydb.c | 5 ++++-
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a56b21a..ebe4e18 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -300,13 +300,14 @@ extern int ss_initialized;
/* The file system's label must be initialized prior to use. */
-static char *labeling_behaviors[6] = {
+static char *labeling_behaviors[7] = {
"uses xattr",
"uses transition SIDs",
"uses task SIDs",
"uses genfs_contexts",
"not configured for labeling",
"uses mountpoint labeling",
+ "uses native labels",
};
static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dentry);
@@ -660,14 +661,15 @@ static int selinux_set_mnt_opts(struct super_block *sb, char **mount_options,
if (strcmp(sb->s_type->name, "proc") == 0)
sbsec->proc = 1;
- /* Determine the labeling behavior to use for this filesystem type. */
- rc = security_fs_use(sb->s_type->name, &sbsec->behavior, &sbsec->sid);
- if (rc) {
- printk(KERN_WARNING "%s: security_fs_use(%s) returned %d\n",
- __FUNCTION__, sb->s_type->name, rc);
- goto out;
+ if (!sbsec->behavior) {
+ /* Determine the labeling behavior to use for this filesystem type. */
+ rc = security_fs_use(sb->s_type->name, &sbsec->behavior, &sbsec->sid);
+ if (rc) {
+ printk(KERN_WARNING "%s: security_fs_use(%s) returned %d\n",
+ __FUNCTION__, sb->s_type->name, rc);
+ goto out;
+ }
}
-
/* sets the context of the superblock for the fs being mounted. */
if (fscontext_sid) {
@@ -1103,6 +1105,8 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
}
switch (sbsec->behavior) {
+ case SECURITY_FS_USE_NATIVE:
+ break;
case SECURITY_FS_USE_XATTR:
if (!inode->i_op->getxattr) {
isec->sid = sbsec->def_sid;
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 837ce42..e788c7a 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -114,6 +114,8 @@ int security_get_allow_unknown(void);
#define SECURITY_FS_USE_GENFS 4 /* use the genfs support */
#define SECURITY_FS_USE_NONE 5 /* no labeling support */
#define SECURITY_FS_USE_MNTPOINT 6 /* use mountpoint labeling */
+#define SECURITY_FS_USE_NATIVE 7 /* use native label support */
+#define SECURITY_FS_USE_MAX 7 /* Highest SECURITY_FS_USE_XXX */
int security_fs_use(const char *fstype, unsigned int *behavior,
u32 *sid);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index bd7d6a0..3d9e30e 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1756,7 +1756,10 @@ int policydb_read(struct policydb *p, void *fp)
if (rc < 0)
goto bad;
c->v.behavior = le32_to_cpu(buf[0]);
- if (c->v.behavior > SECURITY_FS_USE_NONE)
+ /* Determined at runtime, not in policy DB. */
+ if (c->v.behavior == SECURITY_FS_USE_MNTPOINT)
+ goto bad;
+ if (c->v.behavior > SECURITY_FS_USE_MAX)
goto bad;
len = le32_to_cpu(buf[1]);
c->u.name = kmalloc(len + 1,GFP_KERNEL);
--
1.5.3.8
This patch adds two entries into the fs/KConfig file. The first entry
NFS_V4_SECURITY_LABEL enables security label support for the NFSv4 client while
the second entry NFSD_V4_SECURITY_LABEL enables security labeling support on
the server side. These entries are currently marked as EXPERIMENTAL to indicate
that at the moment they are only suitable for testing purposes and that the
functionality they provide may change before it is removed from experimental
status.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/Kconfig | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index d731282..d762375 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -1637,6 +1637,14 @@ config NFS_V4
If unsure, say N.
+config NFS_V4_SECURITY_LABEL
+ bool "Provide Security Label support for NFSv4 client"
+ depends on NFS_V4 && SECURITY && EXPERIMENTAL
+ help
+ Say Y here if you want label attribute support for NFS version 4.
+
+ If unsure, say N.
+
config NFS_DIRECTIO
bool "Allow direct I/O on NFS files"
depends on NFS_FS
@@ -1728,6 +1736,15 @@ config NFSD_V4
should only be used if you are interested in helping to test NFSv4.
If unsure, say N.
+config NFSD_V4_SECURITY_LABEL
+ bool "Provide Security Label support for NFSv4 server"
+ depends on NFSD_V4 && SECURITY && EXPERIMENTAL
+ help
+ If you would like to include support for label file attributes
+ over NFSv4, say Y here.
+
+ If unsure, say N.
+
config NFSD_TCP
bool "Provide NFS server over TCP support"
depends on NFSD
--
1.5.3.8
This patch adds a new recommended attribute named label into the NFSv4 file
attribute structure. It also adds several new flags to allow the
NFS client and server to determine if this attribute is supported and if it is
being sent over the wire.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/nfs/nfs4proc.c | 3 +++
include/linux/nfs4.h | 2 ++
include/linux/nfs_fs_sb.h | 1 +
include/linux/nfs_xdr.h | 4 ++++
include/linux/nfsd/export.h | 5 +++--
include/linux/nfsd/nfsd.h | 7 ++++---
6 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 7ce0786..42e5cf7 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -96,6 +96,9 @@ const u32 nfs4_fattr_bitmap[2] = {
| FATTR4_WORD1_TIME_ACCESS
| FATTR4_WORD1_TIME_METADATA
| FATTR4_WORD1_TIME_MODIFY
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ | FATTR4_WORD1_SECURITY_LABEL
+#endif
};
const u32 nfs4_statfs_bitmap[2] = {
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 8726491..af90403 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -21,6 +21,7 @@
#define NFS4_FHSIZE 128
#define NFS4_MAXPATHLEN PATH_MAX
#define NFS4_MAXNAMLEN NAME_MAX
+#define NFS4_MAXLABELLEN 255
#define NFS4_ACCESS_READ 0x0001
#define NFS4_ACCESS_LOOKUP 0x0002
@@ -348,6 +349,7 @@ enum lock_type4 {
#define FATTR4_WORD1_TIME_MODIFY (1UL << 21)
#define FATTR4_WORD1_TIME_MODIFY_SET (1UL << 22)
#define FATTR4_WORD1_MOUNTED_ON_FILEID (1UL << 23)
+#define FATTR4_WORD1_SECURITY_LABEL (1UL << 31)
#define NFSPROC4_NULL 0
#define NFSPROC4_COMPOUND 1
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index 3423c67..8fdea18 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -125,5 +125,6 @@ struct nfs_server {
#define NFS_CAP_SYMLINKS (1U << 2)
#define NFS_CAP_ACLS (1U << 3)
#define NFS_CAP_ATOMIC_OPEN (1U << 4)
+#define NFS_CAP_SECURITY_LABEL (1U << 5)
#endif
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index f301d0b..d7ce8b9 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -56,6 +56,10 @@ struct nfs_fattr {
__u64 change_attr; /* NFSv4 change attribute */
__u64 pre_change_attr;/* pre-op NFSv4 change attribute */
unsigned long time_start;
+#ifdef CONFIG_SECURITY
+ void *label;
+ __u32 label_len;
+#endif
};
#define NFS_ATTR_WCC 0x0001 /* pre-op WCC data */
diff --git a/include/linux/nfsd/export.h b/include/linux/nfsd/export.h
index 5431512..bb831fc 100644
--- a/include/linux/nfsd/export.h
+++ b/include/linux/nfsd/export.h
@@ -32,7 +32,8 @@
#define NFSEXP_ALLSQUASH 0x0008
#define NFSEXP_ASYNC 0x0010
#define NFSEXP_GATHERED_WRITES 0x0020
-/* 40 80 100 currently unused */
+#define NFSEXP_SECURITY_LABEL 0x0040 /* Support security label fattr4 */
+/* 80 100 currently unused */
#define NFSEXP_NOHIDE 0x0200
#define NFSEXP_NOSUBTREECHECK 0x0400
#define NFSEXP_NOAUTHNLM 0x0800 /* Don't authenticate NLM requests - just trust */
@@ -40,7 +41,7 @@
#define NFSEXP_FSID 0x2000
#define NFSEXP_CROSSMOUNT 0x4000
#define NFSEXP_NOACL 0x8000 /* reserved for possible ACL related use */
-#define NFSEXP_ALLFLAGS 0xFE3F
+#define NFSEXP_ALLFLAGS 0xFE7F
/* The flags that may vary depending on security flavor: */
#define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
diff --git a/include/linux/nfsd/nfsd.h b/include/linux/nfsd/nfsd.h
index 8caf4c4..045963c 100644
--- a/include/linux/nfsd/nfsd.h
+++ b/include/linux/nfsd/nfsd.h
@@ -310,8 +310,8 @@ extern struct timeval nfssvc_boot;
| FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP | FATTR4_WORD1_RAWDEV \
| FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE | FATTR4_WORD1_SPACE_TOTAL \
| FATTR4_WORD1_SPACE_USED | FATTR4_WORD1_TIME_ACCESS | FATTR4_WORD1_TIME_ACCESS_SET \
- | FATTR4_WORD1_TIME_DELTA | FATTR4_WORD1_TIME_METADATA \
- | FATTR4_WORD1_TIME_MODIFY | FATTR4_WORD1_TIME_MODIFY_SET | FATTR4_WORD1_MOUNTED_ON_FILEID)
+ | FATTR4_WORD1_TIME_DELTA | FATTR4_WORD1_TIME_METADATA | FATTR4_WORD1_TIME_MODIFY \
+ | FATTR4_WORD1_TIME_MODIFY_SET | FATTR4_WORD1_MOUNTED_ON_FILEID | FATTR4_WORD1_SECURITY_LABEL)
/* These will return ERR_INVAL if specified in GETATTR or READDIR. */
#define NFSD_WRITEONLY_ATTRS_WORD1 \
@@ -322,7 +322,8 @@ extern struct timeval nfssvc_boot;
(FATTR4_WORD0_SIZE | FATTR4_WORD0_ACL )
#define NFSD_WRITEABLE_ATTRS_WORD1 \
(FATTR4_WORD1_MODE | FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP \
- | FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_METADATA | FATTR4_WORD1_TIME_MODIFY_SET)
+ | FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_METADATA | FATTR4_WORD1_TIME_MODIFY_SET \
+ | FATTR4_WORD1_SECURITY_LABEL)
#endif /* CONFIG_NFSD_V4 */
--
1.5.3.8
Before the inode_xattr_getsecurity call was removed the caller would
concatenate the security namespace prefix and the suffix provided by the lsm to
obtain the security xattr. This hook provides the functionality to obtain the full
LSM xattr name. The patch also provides implementations for the dummy security
module and SELinux. This method is used instead of restoring the old method
since it only requires an offset into the returned pointer to obtain the
suffix. This approach is more efficient than concatenating the security xattr
namespace string with the suffix to get a usable string.
Signed-off-by: David P. Quigley <[email protected]>
---
include/linux/security.h | 8 ++++++++
security/dummy.c | 6 ++++++
security/security.c | 6 ++++++
security/selinux/hooks.c | 10 ++++++++--
4 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index fe52cde..c80bee4 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1394,6 +1394,7 @@ struct security_operations {
int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
int (*secctx_to_secid)(char *secdata, u32 seclen, u32 *secid);
void (*release_secctx)(char *secdata, u32 seclen);
+ const char *(*maclabel_getname) (void);
#ifdef CONFIG_SECURITY_NETWORK
int (*unix_stream_connect) (struct socket * sock,
@@ -1633,6 +1634,7 @@ int security_netlink_recv(struct sk_buff *skb, int cap);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
int security_secctx_to_secid(char *secdata, u32 seclen, u32 *secid);
void security_release_secctx(char *secdata, u32 seclen);
+const char *security_maclabel_getname(void);
#else /* CONFIG_SECURITY */
@@ -2316,6 +2318,12 @@ static inline int security_secctx_to_secid(char *secdata,
static inline void security_release_secctx(char *secdata, u32 seclen)
{
}
+
+static inline const char *security_maclabel_getname(void)
+{
+ return NULL;
+}
+
#endif /* CONFIG_SECURITY */
#ifdef CONFIG_SECURITY_NETWORK
diff --git a/security/dummy.c b/security/dummy.c
index 649326b..928ef41 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -960,6 +960,11 @@ static void dummy_release_secctx(char *secdata, u32 seclen)
{
}
+static const char *dummy_maclabel_getname(void)
+{
+ return NULL;
+}
+
#ifdef CONFIG_KEYS
static inline int dummy_key_alloc(struct key *key, struct task_struct *ctx,
unsigned long flags)
@@ -1118,6 +1123,7 @@ void security_fixup_ops (struct security_operations *ops)
set_to_dummy_if_null(ops, secid_to_secctx);
set_to_dummy_if_null(ops, secctx_to_secid);
set_to_dummy_if_null(ops, release_secctx);
+ set_to_dummy_if_null(ops, maclabel_getname);
#ifdef CONFIG_SECURITY_NETWORK
set_to_dummy_if_null(ops, unix_stream_connect);
set_to_dummy_if_null(ops, unix_may_send);
diff --git a/security/security.c b/security/security.c
index d15e56c..1a84eb1 100644
--- a/security/security.c
+++ b/security/security.c
@@ -845,6 +845,12 @@ void security_release_secctx(char *secdata, u32 seclen)
}
EXPORT_SYMBOL(security_release_secctx);
+const char *security_maclabel_getname(void)
+{
+ return security_ops->maclabel_getname();
+}
+EXPORT_SYMBOL(security_maclabel_getname);
+
#ifdef CONFIG_SECURITY_NETWORK
int security_unix_stream_connect(struct socket *sock, struct socket *other,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 75c2e99..e7fc9c9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5163,6 +5163,11 @@ static void selinux_release_secctx(char *secdata, u32 seclen)
kfree(secdata);
}
+static const char *selinux_maclabel_getname(void)
+{
+ return XATTR_NAME_SELINUX;
+}
+
#ifdef CONFIG_KEYS
static int selinux_key_alloc(struct key *k, struct task_struct *tsk,
@@ -5351,8 +5356,9 @@ static struct security_operations selinux_ops = {
.secid_to_secctx = selinux_secid_to_secctx,
.secctx_to_secid = selinux_secctx_to_secid,
.release_secctx = selinux_release_secctx,
-
- .unix_stream_connect = selinux_socket_unix_stream_connect,
+ .maclabel_getname = selinux_maclabel_getname,
+
+ .unix_stream_connect = selinux_socket_unix_stream_connect,
.unix_may_send = selinux_socket_unix_may_send,
.socket_create = selinux_socket_create,
--
1.5.3.8
The new method for pulling argument for NFS from mount is through a text
parsing system. This patch adds two new entries to the argument parsing code
"securlty_label" and "nosecurity_label". Even though we use text across the
user/kernel boundary internally we still pack a binary structure for mount info
to be passed around. We add a flag for use in the nfs{4,}_mount_data struct to
indicate that are using security labels. Finally we add the SELinux support to
mark the labeling method as native.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/nfs/super.c | 10 +++++++++-
fs/nfsd/export.c | 3 +++
include/linux/nfs4_mount.h | 3 ++-
include/linux/nfs_mount.h | 3 ++-
security/selinux/hooks.c | 13 ++++++++++++-
5 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 1fb3818..f3e327e 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -75,6 +75,7 @@ enum {
Opt_acl, Opt_noacl,
Opt_rdirplus, Opt_nordirplus,
Opt_sharecache, Opt_nosharecache,
+ Opt_seclabel, Opt_noseclabel,
/* Mount options that take integer arguments */
Opt_port,
@@ -124,6 +125,8 @@ static match_table_t nfs_mount_option_tokens = {
{ Opt_nordirplus, "nordirplus" },
{ Opt_sharecache, "sharecache" },
{ Opt_nosharecache, "nosharecache" },
+ { Opt_seclabel, "security_label" },
+ { Opt_noseclabel, "nosecurity_label" },
{ Opt_port, "port=%u" },
{ Opt_rsize, "rsize=%u" },
@@ -779,7 +782,12 @@ static int nfs_parse_mount_options(char *raw,
case Opt_nosharecache:
mnt->flags |= NFS_MOUNT_UNSHARED;
break;
-
+ case Opt_seclabel:
+ mnt->flags |= NFS_MOUNT_SECURITY_LABEL;
+ break;
+ case Opt_noseclabel:
+ mnt->flags &= ~NFS_MOUNT_SECURITY_LABEL;
+ break;
case Opt_port:
if (match_int(args, &option))
return 0;
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 8a6f7c9..d32ae56 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1435,6 +1435,9 @@ static struct flags {
{ NFSEXP_ALLSQUASH, {"all_squash", ""}},
{ NFSEXP_ASYNC, {"async", "sync"}},
{ NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}},
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+ { NFSEXP_SECURITY_LABEL, {"security_label", ""}},
+#endif
{ NFSEXP_NOHIDE, {"nohide", ""}},
{ NFSEXP_CROSSMOUNT, {"crossmnt", ""}},
{ NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}},
diff --git a/include/linux/nfs4_mount.h b/include/linux/nfs4_mount.h
index a0dcf66..d7abc3b 100644
--- a/include/linux/nfs4_mount.h
+++ b/include/linux/nfs4_mount.h
@@ -66,6 +66,7 @@ struct nfs4_mount_data {
#define NFS4_MOUNT_NOAC 0x0020 /* 1 */
#define NFS4_MOUNT_STRICTLOCK 0x1000 /* 1 */
#define NFS4_MOUNT_UNSHARED 0x8000 /* 1 */
-#define NFS4_MOUNT_FLAGMASK 0x9033
+#define NFS4_MOUNT_SECURITY_LABEL 0x10000 /* Text Only */
+#define NFS4_MOUNT_FLAGMASK 0x19033
#endif
diff --git a/include/linux/nfs_mount.h b/include/linux/nfs_mount.h
index df7c6b7..1ca5260 100644
--- a/include/linux/nfs_mount.h
+++ b/include/linux/nfs_mount.h
@@ -63,6 +63,7 @@ struct nfs_mount_data {
#define NFS_MOUNT_SECFLAVOUR 0x2000 /* 5 */
#define NFS_MOUNT_NORDIRPLUS 0x4000 /* 5 */
#define NFS_MOUNT_UNSHARED 0x8000 /* 5 */
-#define NFS_MOUNT_FLAGMASK 0xFFFF
+#define NFS_MOUNT_SECURITY_LABEL 0x10000 /* reserved for NFSv4 */
+#define NFS_MOUNT_FLAGMASK 0x1FFFF
#endif
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ebe4e18..e3ed7c3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -68,6 +68,7 @@
#include <net/af_unix.h> /* for Unix socket types */
#include <linux/parser.h>
#include <linux/nfs_mount.h>
+#include <linux/nfs4_mount.h>
#include <net/ipv6.h>
#include <linux/hugetlb.h>
#include <linux/personality.h>
@@ -807,6 +808,7 @@ static int superblock_doinit(struct super_block *sb, void *data)
/* selinux only know about a fixed number of mount options */
char *mnt_opts[NUM_SEL_MNT_OPTS];
int mnt_opts_flags[NUM_SEL_MNT_OPTS], num_mnt_opts = 0;
+ struct superblock_security_struct *sbsec = sb->s_security;
if (!data)
goto out;
@@ -829,6 +831,15 @@ static int superblock_doinit(struct super_block *sb, void *data)
}
}
goto build_flags;
+ } else if (!strcmp(name, "nfs4")) {
+ struct nfs4_mount_data *d = data;
+
+ if (d->version != NFS4_MOUNT_VERSION)
+ goto out;
+
+ if (d->flags & NFS4_MOUNT_SECURITY_LABEL)
+ sbsec->behavior = SECURITY_FS_USE_NATIVE;
+ goto build_flags;
} else
goto out;
}
@@ -898,7 +909,7 @@ static int superblock_doinit(struct super_block *sb, void *data)
default:
rc = -EINVAL;
- printk(KERN_WARNING "SELinux: unknown mount option\n");
+ printk(KERN_WARNING "SELinux: unknown mount option \"%s\"\n", p);
goto out_err;
}
--
1.5.3.8
There is a time where we need to calculate a context without the
inode having been created yet. To do this we take the negative dentry and
calculate a context based on the process and the parent directory contexts.
Signed-off-by: David P. Quigley <[email protected]>
---
include/linux/security.h | 11 +++++++++++
security/dummy.c | 7 +++++++
security/security.c | 9 +++++++++
security/selinux/hooks.c | 38 ++++++++++++++++++++++++++++++++++++++
4 files changed, 65 insertions(+), 0 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index c80bee4..9038f34 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1264,6 +1264,8 @@ struct security_operations {
void (*sb_clone_mnt_opts) (const struct super_block *oldsb,
struct super_block *newsb);
+ int (*dentry_init_security) (struct dentry *dentry, int mode,
+ void **ctx, u32 *ctxlen);
int (*inode_alloc_security) (struct inode *inode);
void (*inode_free_security) (struct inode *inode);
int (*inode_init_security) (struct inode *inode, struct inode *dir,
@@ -1528,6 +1530,7 @@ int security_sb_set_mnt_opts(struct super_block *sb, char **mount_options,
void security_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb);
+int security_dentry_init_security(struct dentry *dentry, int mode, void **ctx, u32 *ctxlen);
int security_inode_alloc(struct inode *inode);
void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
@@ -1822,6 +1825,14 @@ static inline void security_sb_post_pivotroot (struct nameidata *old_nd,
struct nameidata *new_nd)
{ }
+static inline int security_dentry_init_security(struct dentry *dentry,
+ int mode,
+ void **ctx,
+ u32 *ctxlen)
+{
+ return 0;
+}
+
static inline int security_inode_alloc (struct inode *inode)
{
return 0;
diff --git a/security/dummy.c b/security/dummy.c
index 928ef41..d322c73 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -268,6 +268,12 @@ static void dummy_sb_clone_mnt_opts(const struct super_block *oldsb,
return;
}
+static int dummy_dentry_init_security(struct dentry *dentry, int mode,
+ void **ctx, u32 *ctxlen)
+{
+ return -EOPNOTSUPP;
+}
+
static int dummy_inode_alloc_security (struct inode *inode)
{
return 0;
@@ -1033,6 +1039,7 @@ void security_fixup_ops (struct security_operations *ops)
set_to_dummy_if_null(ops, sb_get_mnt_opts);
set_to_dummy_if_null(ops, sb_set_mnt_opts);
set_to_dummy_if_null(ops, sb_clone_mnt_opts);
+ set_to_dummy_if_null(ops, dentry_init_security);
set_to_dummy_if_null(ops, inode_alloc_security);
set_to_dummy_if_null(ops, inode_free_security);
set_to_dummy_if_null(ops, inode_init_security);
diff --git a/security/security.c b/security/security.c
index 1a84eb1..b6e80bb 100644
--- a/security/security.c
+++ b/security/security.c
@@ -325,6 +325,15 @@ void security_sb_clone_mnt_opts(const struct super_block *oldsb,
security_ops->sb_clone_mnt_opts(oldsb, newsb);
}
+int security_dentry_init_security(struct dentry *dentry,
+ int mode,
+ void **ctx,
+ u32 *ctxlen)
+{
+ return security_ops->dentry_init_security(dentry, mode, ctx, ctxlen);
+}
+EXPORT_SYMBOL(security_dentry_init_security);
+
int security_inode_alloc(struct inode *inode)
{
inode->i_security = NULL;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7fc9c9..a56b21a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -63,6 +63,7 @@
#include <linux/udp.h>
#include <linux/dccp.h>
#include <linux/quota.h>
+#include <linux/fsnotify.h>
#include <linux/un.h> /* for Unix socket types */
#include <net/af_unix.h> /* for Unix socket types */
#include <linux/parser.h>
@@ -2358,6 +2359,42 @@ static int selinux_umount(struct vfsmount *mnt, int flags)
/* inode security operations */
+/*
+ * For now, we need a way to compute a SID for
+ * a dentry as the inode is not yet available
+ * (and under NFSv4 has no label backed by an EA anyway.
+ */
+static int selinux_dentry_init_security(struct dentry *dentry, int mode,
+ void **ctx, u32 *ctxlen)
+{
+ struct task_security_struct *tsec;
+ struct inode_security_struct *dsec;
+ struct superblock_security_struct *sbsec;
+ struct inode *dir = dentry->d_parent->d_inode;
+ u32 newsid;
+ int rc;
+
+ tsec = current->security;
+ dsec = dir->i_security;
+ sbsec = dir->i_sb->s_security;
+
+ if (tsec->create_sid && sbsec->behavior != SECURITY_FS_USE_MNTPOINT) {
+ newsid = tsec->create_sid;
+ } else {
+ rc = security_transition_sid(tsec->sid, dsec->sid,
+ inode_mode_to_security_class(mode),
+ &newsid);
+ if (rc) {
+ printk(KERN_WARNING "%s: "
+ "security_transition_sid failed, rc=%d\n",
+ __FUNCTION__, -rc);
+ return rc;
+ }
+ }
+
+ return security_sid_to_context(newsid, (char **)ctx, ctxlen);
+}
+
static int selinux_inode_alloc_security(struct inode *inode)
{
return inode_alloc_security(inode);
@@ -5257,6 +5294,7 @@ static struct security_operations selinux_ops = {
.sb_set_mnt_opts = selinux_set_mnt_opts,
.sb_clone_mnt_opts = selinux_sb_clone_mnt_opts,
+ .dentry_init_security = selinux_dentry_init_security,
.inode_alloc_security = selinux_inode_alloc_security,
.inode_free_security = selinux_inode_free_security,
.inode_init_security = selinux_inode_init_security,
--
1.5.3.8
This patch implements the encoding of a MAC label on the server side to be
sent across the wire to the NFSv4 client. At this time there is no method of
receiving a label from the client to be set on the server.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/nfsd/nfs4xdr.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++-
fs/nfsd/vfs.c | 7 ++++
security/security.c | 1 +
3 files changed, 97 insertions(+), 1 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 0e6a179..5de2a91 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -50,6 +50,7 @@
#include <linux/sunrpc/xdr.h>
#include <linux/sunrpc/svc.h>
#include <linux/sunrpc/clnt.h>
+#include <linux/nfs_fs.h>
#include <linux/nfsd/nfsd.h>
#include <linux/nfsd/state.h>
#include <linux/nfsd/xdr4.h>
@@ -58,6 +59,8 @@
#include <linux/nfs4_acl.h>
#include <linux/sunrpc/gss_api.h>
#include <linux/sunrpc/svcauth_gss.h>
+#include <linux/security.h>
+#include <linux/xattr.h>
#define NFSDDBG_FACILITY NFSDDBG_XDR
@@ -416,6 +419,22 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval, struct iattr *ia
goto xdr_error;
}
}
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+ if (bmval[1] & FATTR4_WORD1_SECURITY_LABEL) {
+ READ_BUF(4);
+ len += 4;
+ READ32(dummy32);
+ READ_BUF(dummy32);
+ len += (XDR_QUADLEN(dummy32) << 2);
+ READMEM(buf, dummy32);
+ iattr->ia_label_len = dummy32;
+ iattr->ia_label = kmalloc(dummy32 + 1, GFP_ATOMIC);
+ memcpy(iattr->ia_label, buf, dummy32);
+ ((char *)iattr->ia_label)[dummy32 + 1] = '\0';
+ iattr->ia_valid |= ATTR_SECURITY_LABEL;
+ defer_free(argp, kfree, iattr->ia_label);
+ }
+#endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
if (len != expected_len)
goto xdr_error;
@@ -1423,6 +1442,44 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, int whotype, uid_t id, int group,
return nfsd4_encode_name(rqstp, whotype, id, group, p, buflen);
}
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+static inline __be32
+nfsd4_encode_security_label(struct svc_rqst *rqstp,
+ struct dentry *dentry,
+ __be32 **p, int *buflen)
+{
+ void *context;
+ int err = 0, len;
+
+ const char *suffix = security_maclabel_getname()
+ + XATTR_SECURITY_PREFIX_LEN;
+
+ len = security_inode_getsecurity(dentry->d_inode, suffix, &context, true);
+ if (len < 0) {
+ err = nfserrno(len);
+ goto out;
+ }
+
+ if (len > NFS4_MAXLABELLEN) {
+ err = nfserrno(len);
+ goto out_err;
+ }
+ if (*buflen < ((XDR_QUADLEN(len) << 2) + 4)) {
+ err = nfserr_resource;
+ goto out_err;
+ }
+
+ *p = xdr_encode_opaque(*p, context, len);
+ *buflen -= (XDR_QUADLEN(len) << 2) + 4;
+ BUG_ON(*buflen < 0);
+
+out_err:
+ security_release_secctx(context, len);
+out:
+ return err;
+}
+#endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
+
#define WORD0_ABSENT_FS_ATTRS (FATTR4_WORD0_FS_LOCATIONS | FATTR4_WORD0_FSID | \
FATTR4_WORD0_RDATTR_ERROR)
#define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
@@ -1518,6 +1575,16 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
bmval0 &= ~FATTR4_WORD0_FS_LOCATIONS;
}
}
+
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+ if (bmval1 & FATTR4_WORD1_SECURITY_LABEL) {
+ if (/* XXX !selinux_enabled */0)
+ bmval1 &= ~FATTR4_WORD1_SECURITY_LABEL;
+ }
+#else
+ bmval1 &= ~FATTR4_WORD1_SECURITY_LABEL;
+#endif
+
if ((buflen -= 16) < 0)
goto out_resource;
@@ -1528,15 +1595,25 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
if (bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
u32 word0 = NFSD_SUPPORTED_ATTRS_WORD0;
+ u32 word1 = NFSD_SUPPORTED_ATTRS_WORD1;
if ((buflen -= 12) < 0)
goto out_resource;
if (!aclsupport)
word0 &= ~FATTR4_WORD0_ACL;
if (!exp->ex_fslocs.locations)
word0 &= ~FATTR4_WORD0_FS_LOCATIONS;
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+ if (exp->ex_flags & NFSEXP_SECURITY_LABEL)
+ word1 |= FATTR4_WORD1_SECURITY_LABEL;
+ else
+ word1 &= ~FATTR4_WORD1_SECURITY_LABEL;
+#else
+ word1 &= ~FATTR4_WORD1_SECURITY_LABEL;
+#endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
+
WRITE32(2);
WRITE32(word0);
- WRITE32(NFSD_SUPPORTED_ATTRS_WORD1);
+ WRITE32(word1);
}
if (bmval0 & FATTR4_WORD0_TYPE) {
if ((buflen -= 4) < 0)
@@ -1846,6 +1923,17 @@ out_acl:
}
WRITE64(stat.ino);
}
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+ if (bmval1 & FATTR4_WORD1_SECURITY_LABEL) {
+ status = nfsd4_encode_security_label(rqstp, dentry,
+ &p, &buflen);
+ if (status == nfserr_resource)
+ goto out_resource;
+ if (status)
+ goto out;
+ }
+#endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
+
*attrlenp = htonl((char *)p - (char *)attrlenp - 4);
*countp = p - buffer;
status = nfs_ok;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 46f59d5..45e9340 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1538,6 +1538,13 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
if (!host_err) {
if (EX_ISSYNC(fhp->fh_export))
host_err = nfsd_sync_dir(dentry);
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+ if (iap && (iap->ia_valid & ATTR_SECURITY_LABEL)) {
+ char *key = (char *)security_maclabel_getname();
+ host_err = vfs_setxattr_locked(dnew, key,
+ iap->ia_label, iap->ia_label_len, 0);
+ }
+#endif
}
err = nfserrno(host_err);
fh_unlock(fhp);
diff --git a/security/security.c b/security/security.c
index 1276c98..646a411 100644
--- a/security/security.c
+++ b/security/security.c
@@ -510,6 +510,7 @@ int security_inode_getsecurity(const struct inode *inode, const char *name, void
return 0;
return security_ops->inode_getsecurity(inode, name, buffer, alloc);
}
+EXPORT_SYMBOL(security_inode_getsecurity);
int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
{
--
1.5.3.8
This patch adds two new fields to the iattr structure. The first field holds a
security label while the second contains the length of this label. In addition
the patch adds a new helper function inode_setsecurity which calls the LSM to
set the security label on the inode. Finally the patch modifies the necessary
functions such that fsnotify_change can handle notification requests for
dnotify and inotify.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/attr.c | 43 +++++++++++++++++++++++++++++++++++++++++++
fs/xattr.c | 33 +++++++++++++++++++++++++++------
include/linux/fcntl.h | 1 +
include/linux/fs.h | 11 +++++++++++
include/linux/fsnotify.h | 6 ++++++
include/linux/inotify.h | 3 ++-
include/linux/xattr.h | 1 +
7 files changed, 91 insertions(+), 7 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index 966b73e..1df6603 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -5,6 +5,7 @@
* changes by Thomas Schoebel-Theuer
*/
+#include <linux/fs.h>
#include <linux/module.h>
#include <linux/time.h>
#include <linux/mm.h>
@@ -14,9 +15,35 @@
#include <linux/fcntl.h>
#include <linux/quotaops.h>
#include <linux/security.h>
+#include <linux/xattr.h>
/* Taken over from the old code... */
+#ifdef CONFIG_SECURITY
+/*
+ * Update the in core label.
+ */
+int inode_setsecurity(struct inode *inode, struct iattr *attr)
+{
+ const char *suffix = security_maclabel_getname()
+ + XATTR_SECURITY_PREFIX_LEN;
+ int error;
+
+ if (!attr->ia_valid & ATTR_SECURITY_LABEL)
+ return -EINVAL;
+
+ error = security_inode_setsecurity(inode, suffix, attr->ia_label,
+ attr->ia_label_len, 0);
+ if (error)
+ printk(KERN_ERR "%s() %s %d security_inode_setsecurity() %d\n"
+ , __func__, (char *)attr->ia_label, attr->ia_label_len
+ , error);
+
+ return error;
+}
+EXPORT_SYMBOL(inode_setsecurity);
+#endif
+
/* POSIX UID/GID verification for setting inode attributes. */
int inode_change_ok(struct inode *inode, struct iattr *attr)
{
@@ -94,6 +121,10 @@ int inode_setattr(struct inode * inode, struct iattr * attr)
mode &= ~S_ISGID;
inode->i_mode = mode;
}
+#ifdef CONFIG_SECURITY
+ if (ia_valid & ATTR_SECURITY_LABEL)
+ inode_setsecurity(inode, attr);
+#endif
mark_inode_dirty(inode);
return 0;
@@ -157,6 +188,18 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
if (ia_valid & ATTR_SIZE)
down_write(&dentry->d_inode->i_alloc_sem);
+#ifdef CONFIG_SECURITY
+ if (ia_valid & ATTR_SECURITY_LABEL) {
+ char *key = (char *)security_maclabel_getname();
+ vfs_setxattr_locked(dentry, key,
+ attr->ia_label, attr->ia_label_len, 0);
+ /* Avoid calling inode_setsecurity()
+ * via inode_setattr() below
+ */
+ attr->ia_valid &= ~ATTR_SECURITY_LABEL;
+ }
+#endif
+
if (inode->i_op && inode->i_op->setattr) {
error = security_inode_setattr(dentry, attr);
if (!error)
diff --git a/fs/xattr.c b/fs/xattr.c
index 3acab16..b5a91e1 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -67,9 +67,9 @@ xattr_permission(struct inode *inode, const char *name, int mask)
return permission(inode, mask, NULL);
}
-int
-vfs_setxattr(struct dentry *dentry, char *name, void *value,
- size_t size, int flags)
+static int
+_vfs_setxattr(struct dentry *dentry, char *name, void *value,
+ size_t size, int flags, int lock)
{
struct inode *inode = dentry->d_inode;
int error;
@@ -78,7 +78,8 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
if (error)
return error;
- mutex_lock(&inode->i_mutex);
+ if (lock)
+ mutex_lock(&inode->i_mutex);
error = security_inode_setxattr(dentry, name, value, size, flags);
if (error)
goto out;
@@ -95,15 +96,35 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
error = security_inode_setsecurity(inode, suffix, value,
size, flags);
- if (!error)
+ if (!error) {
+#ifdef CONFIG_SECURITY
+ fsnotify_change(dentry, ATTR_SECURITY_LABEL);
+#endif
fsnotify_xattr(dentry);
+ }
}
out:
- mutex_unlock(&inode->i_mutex);
+ if (lock)
+ mutex_unlock(&inode->i_mutex);
return error;
}
+
+int
+vfs_setxattr(struct dentry *dentry, char *name, void *value,
+ size_t size, int flags)
+{
+ return _vfs_setxattr(dentry, name, value, size, flags, 1);
+}
EXPORT_SYMBOL_GPL(vfs_setxattr);
+int
+vfs_setxattr_locked(struct dentry *dentry, char *name, void *value,
+ size_t size, int flags)
+{
+ return _vfs_setxattr(dentry, name, value, size, flags, 0);
+}
+EXPORT_SYMBOL_GPL(vfs_setxattr_locked);
+
ssize_t
xattr_getsecurity(struct inode *inode, const char *name, void *value,
size_t size)
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 8603740..fae1e59 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -30,6 +30,7 @@
#define DN_DELETE 0x00000008 /* File removed */
#define DN_RENAME 0x00000010 /* File renamed */
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
+#define DN_LABEL 0x00000040 /* File (re)labeled */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
#define AT_FDCWD -100 /* Special value used to indicate
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b84b848..757add6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -335,6 +335,10 @@ typedef void (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define ATTR_KILL_PRIV 16384
#define ATTR_OPEN 32768 /* Truncating from open(O_TRUNC) */
+#ifdef CONFIG_SECURITY
+#define ATTR_SECURITY_LABEL 65536
+#endif
+
/*
* This is the Inode Attributes structure, used for notify_change(). It
* uses the above definitions as flags, to know which values have changed.
@@ -360,6 +364,10 @@ struct iattr {
* check for (ia_valid & ATTR_FILE), and not for (ia_file != NULL).
*/
struct file *ia_file;
+#ifdef CONFIG_SECURITY
+ void *ia_label;
+ u32 ia_label_len;
+#endif
};
/*
@@ -1971,6 +1979,9 @@ extern int buffer_migrate_page(struct address_space *,
#define buffer_migrate_page NULL
#endif
+#ifdef CONFIG_SECURITY
+extern int inode_setsecurity(struct inode *inode, struct iattr *attr);
+#endif
extern int inode_change_ok(struct inode *, struct iattr *);
extern int __must_check inode_setattr(struct inode *, struct iattr *);
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index d4b7c4a..b54a3cb 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -253,6 +253,12 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
in_mask |= IN_ATTRIB;
dn_mask |= DN_ATTRIB;
}
+#ifdef CONFIG_SECURITY
+ if (ia_valid & ATTR_SECURITY_LABEL) {
+ in_mask |= IN_LABEL;
+ dn_mask |= DN_LABEL;
+ }
+#endif
if (dn_mask)
dnotify_parent(dentry, dn_mask);
diff --git a/include/linux/inotify.h b/include/linux/inotify.h
index 742b917..10f3ace 100644
--- a/include/linux/inotify.h
+++ b/include/linux/inotify.h
@@ -36,6 +36,7 @@ struct inotify_event {
#define IN_DELETE 0x00000200 /* Subfile was deleted */
#define IN_DELETE_SELF 0x00000400 /* Self was deleted */
#define IN_MOVE_SELF 0x00000800 /* Self was moved */
+#define IN_LABEL 0x00001000 /* Self was (re)labeled */
/* the following are legal events. they are sent as needed to any watch */
#define IN_UNMOUNT 0x00002000 /* Backing fs was unmounted */
@@ -61,7 +62,7 @@ struct inotify_event {
#define IN_ALL_EVENTS (IN_ACCESS | IN_MODIFY | IN_ATTRIB | IN_CLOSE_WRITE | \
IN_CLOSE_NOWRITE | IN_OPEN | IN_MOVED_FROM | \
IN_MOVED_TO | IN_DELETE | IN_CREATE | IN_DELETE_SELF | \
- IN_MOVE_SELF)
+ IN_MOVE_SELF | IN_LABEL)
#ifdef __KERNEL__
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index df6b95d..1169963 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -50,6 +50,7 @@ ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
+int vfs_setxattr_locked(struct dentry *, char *, void *, size_t, int);
int vfs_removexattr(struct dentry *, char *);
ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
--
1.5.3.8
There are several places where recommended attributes are implemented in the
NFSv4 client code. This patch adds two functions to encode and decode the secid
recommended attribute which makes use of the LSM hooks added earlier. It also
adds code to grab the label from the file attribute structures and encode the
label to be sent back to the server. Even though the code is there to encode a
label to be sent back to the server there does not appear to be an interface to
use it yet.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/nfs/dir.c | 76 ++++++++++++++++++-
fs/nfs/inode.c | 42 ++++++++++-
fs/nfs/nfs4proc.c | 191 +++++++++++++++++++++++++++++++++++++++++++++-
fs/nfs/nfs4xdr.c | 49 ++++++++++++
fs/nfs/super.c | 11 +++
security/security.c | 1 +
security/selinux/hooks.c | 8 ++-
7 files changed, 372 insertions(+), 6 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 19808be..bcb9e52 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -40,6 +40,10 @@
#include "iostat.h"
#include "internal.h"
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+#include <linux/security.h>
+#endif
+
/* #define NFS_DEBUG_VERBOSE 1 */
static int nfs_opendir(struct inode *, struct file *);
@@ -1234,18 +1238,36 @@ static int nfs_create(struct inode *dir, struct dentry *dentry, int mode,
attr.ia_mode = mode;
attr.ia_valid = ATTR_MODE;
- if ((nd->flags & LOOKUP_CREATE) != 0)
+ if ((nd->flags & LOOKUP_CREATE) != 0) {
open_flags = nd->intent.open.flags;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL)) {
+ error = security_dentry_init_security(dentry,
+ attr.ia_mode,
+ &attr.ia_label, &attr.ia_label_len);
+ if (error == 0)
+ attr.ia_valid |= ATTR_SECURITY_LABEL;
+ }
+#endif
+ }
lock_kernel();
error = NFS_PROTO(dir)->create(dir, dentry, &attr, open_flags, nd);
if (error != 0)
goto out_err;
unlock_kernel();
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
return 0;
out_err:
unlock_kernel();
d_drop(dentry);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
return error;
}
@@ -1268,8 +1290,22 @@ nfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
attr.ia_mode = mode;
attr.ia_valid = ATTR_MODE;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL)) {
+ status = security_dentry_init_security(dentry,
+ attr.ia_mode,
+ &attr.ia_label, &attr.ia_label_len);
+ if (status == 0)
+ attr.ia_valid |= ATTR_SECURITY_LABEL;
+ }
+#endif
+
lock_kernel();
status = NFS_PROTO(dir)->mknod(dir, dentry, &attr, rdev);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
if (status != 0)
goto out_err;
unlock_kernel();
@@ -1277,6 +1313,10 @@ nfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
out_err:
unlock_kernel();
d_drop(dentry);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
return status;
}
@@ -1295,15 +1335,31 @@ static int nfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
attr.ia_valid = ATTR_MODE;
attr.ia_mode = mode | S_IFDIR;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL)) {
+ error = security_dentry_init_security(dentry, attr.ia_mode,
+ &attr.ia_label, &attr.ia_label_len);
+ if (error == 0)
+ attr.ia_valid |= ATTR_SECURITY_LABEL;
+ }
+#endif
lock_kernel();
error = NFS_PROTO(dir)->mkdir(dir, dentry, &attr);
if (error != 0)
goto out_err;
unlock_kernel();
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
return 0;
out_err:
d_drop(dentry);
unlock_kernel();
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
return error;
}
@@ -1513,6 +1569,16 @@ static int nfs_symlink(struct inode *dir, struct dentry *dentry, const char *sym
attr.ia_mode = S_IFLNK | S_IRWXUGO;
attr.ia_valid = ATTR_MODE;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL)) {
+ error = security_dentry_init_security(dentry,
+ attr.ia_mode,
+ &attr.ia_label, &attr.ia_label_len);
+ if (error == 0)
+ attr.ia_valid |= ATTR_SECURITY_LABEL;
+ }
+#endif
+
lock_kernel();
page = alloc_page(GFP_HIGHUSER);
@@ -1535,6 +1601,10 @@ static int nfs_symlink(struct inode *dir, struct dentry *dentry, const char *sym
d_drop(dentry);
__free_page(page);
unlock_kernel();
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
return error;
}
@@ -1553,6 +1623,10 @@ static int nfs_symlink(struct inode *dir, struct dentry *dentry, const char *sym
__free_page(page);
unlock_kernel();
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
return 0;
}
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c34fb7c..229b0e8 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -37,6 +37,7 @@
#include <linux/vfs.h>
#include <linux/inet.h>
#include <linux/nfs_xdr.h>
+#include <linux/xattr.h>
#include <asm/system.h>
#include <asm/uaccess.h>
@@ -47,6 +48,10 @@
#include "iostat.h"
#include "internal.h"
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+#include <linux/security.h>
+#endif
+
#define NFSDBG_FACILITY NFSDBG_VFS
#define NFS_64_BIT_INODE_NUMBERS_ENABLED 1
@@ -237,6 +242,27 @@ nfs_init_locked(struct inode *inode, void *opaque)
/* Don't use READDIRPLUS on directories that we believe are too large */
#define NFS_LIMIT_READDIRPLUS (8*PAGE_SIZE)
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+static inline void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr)
+{
+ int error;
+
+ if ((fattr->valid & NFS_ATTR_FATTR_V4) &&
+ (fattr->bitmap[1] & FATTR4_WORD1_SECURITY_LABEL) &&
+ (fattr->label != NULL) &&
+ (inode->i_security != NULL)) {
+ const char *key = security_maclabel_getname() +
+ XATTR_SECURITY_PREFIX_LEN;
+ error = security_inode_setsecurity(inode, key, fattr->label,
+ fattr->label_len, 0);
+ if (error)
+ printk(KERN_ERR
+ "%s() %s %d security_inode_setsecurity() %d\n",
+ __func__, fattr->label, fattr->label_len,
+ error);
+ }
+}
+#endif
/*
* This is our front-end to iget that looks up inodes by file handle
* instead of inode number.
@@ -317,6 +343,11 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr)
inode->i_nlink = fattr->nlink;
inode->i_uid = fattr->uid;
inode->i_gid = fattr->gid;
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ nfs_setsecurity(inode, fattr);
+#endif /* CONFIG_NFS_V4_SECURITY_LABEL */
+
if (fattr->valid & (NFS_ATTR_FATTR_V3 | NFS_ATTR_FATTR_V4)) {
/*
* report the blocks in 512byte units
@@ -346,7 +377,7 @@ out_no_inode:
goto out;
}
-#define NFS_VALID_ATTRS (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_SIZE|ATTR_ATIME|ATTR_ATIME_SET|ATTR_MTIME|ATTR_MTIME_SET)
+#define NFS_VALID_ATTRS (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_SIZE|ATTR_ATIME|ATTR_ATIME_SET|ATTR_MTIME|ATTR_MTIME_SET|ATTR_SECURITY_LABEL)
int
nfs_setattr(struct dentry *dentry, struct iattr *attr)
@@ -425,6 +456,11 @@ void nfs_setattr_update_inode(struct inode *inode, struct iattr *attr)
inode->i_size = attr->ia_size;
vmtruncate(inode, attr->ia_size);
}
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if ((attr->ia_valid & ATTR_SECURITY_LABEL) != 0)
+ inode_setsecurity(inode, attr);
+#endif
}
static int nfs_wait_schedule(void *word)
@@ -1085,6 +1121,10 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
inode->i_uid = fattr->uid;
inode->i_gid = fattr->gid;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ nfs_setsecurity(inode, fattr);
+#endif /* CONFIG_NFS_V4_SECURITY_LABEL */
+
if (fattr->valid & (NFS_ATTR_FATTR_V3 | NFS_ATTR_FATTR_V4)) {
/*
* report the blocks in 512byte units
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b278f7c..a1a4051 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -48,11 +48,18 @@
#include <linux/smp_lock.h>
#include <linux/namei.h>
#include <linux/mount.h>
+#include <linux/xattr.h>
+#include <linux/nfs4_mount.h>
+#include <linux/fsnotify.h>
#include "nfs4_fs.h"
#include "delegation.h"
#include "iostat.h"
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+#include <linux/security.h>
+#endif
+
#define NFSDBG_FACILITY NFSDBG_PROC
#define NFS4_POLL_RETRY_MIN (HZ/10)
@@ -1419,25 +1426,40 @@ nfs4_atomic_open(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
struct nfs4_state *state;
struct dentry *res;
+ cred = rpcauth_lookupcred(NFS_CLIENT(dir)->cl_auth, 0);
+ if (IS_ERR(cred))
+ return (struct dentry *)cred;
+
memset(&attr, 0, sizeof(struct iattr));
if (nd->flags & LOOKUP_CREATE) {
attr.ia_mode = nd->intent.open.create_mode;
attr.ia_valid = ATTR_MODE;
if (!IS_POSIXACL(dir))
attr.ia_mode &= ~current->fs->umask;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL)) {
+ int error;
+ error = security_dentry_init_security(dentry,
+ attr.ia_mode,
+ &attr.ia_label, &attr.ia_label_len);
+ if (error == 0)
+ attr.ia_valid |= ATTR_SECURITY_LABEL;
+ }
+#endif
} else {
attr.ia_valid = 0;
BUG_ON(nd->intent.open.flags & O_CREAT);
}
- cred = rpcauth_lookupcred(NFS_CLIENT(dir)->cl_auth, 0);
- if (IS_ERR(cred))
- return (struct dentry *)cred;
parent = dentry->d_parent;
/* Protect against concurrent sillydeletes */
nfs_block_sillyrename(parent);
state = nfs4_do_open(dir, &path, nd->intent.open.flags, &attr, cred);
put_rpccred(cred);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (attr.ia_label != NULL)
+ security_release_secctx(attr.ia_label, attr.ia_label_len);
+#endif
if (IS_ERR(state)) {
if (PTR_ERR(state) == -ENOENT) {
d_add(dentry, NULL);
@@ -1510,6 +1532,13 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
memcpy(server->attr_bitmask, res.attr_bitmask, sizeof(server->attr_bitmask));
if (res.attr_bitmask[0] & FATTR4_WORD0_ACL)
server->caps |= NFS_CAP_ACLS;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->flags & NFS4_MOUNT_SECURITY_LABEL &&
+ res.attr_bitmask[1] & FATTR4_WORD1_SECURITY_LABEL) {
+ server->caps |= NFS_CAP_SECURITY_LABEL;
+ } else
+#endif
+ server->attr_bitmask[1] &= ~FATTR4_WORD1_SECURITY_LABEL;
if (res.has_links != 0)
server->caps |= NFS_CAP_HARDLINKS;
if (res.has_symlinks != 0)
@@ -2862,6 +2891,162 @@ static int nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t buflen
return err;
}
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+static int _nfs4_get_security_label(struct inode *inode, void *buf, size_t buflen)
+{
+ struct nfs_server *server = NFS_SERVER(inode);
+ struct nfs_fattr fattr;
+ u32 bitmask[2] = { 0, FATTR4_WORD1_SECURITY_LABEL };
+ struct nfs4_getattr_arg args = {
+ .fh = NFS_FH(inode),
+ .bitmask = bitmask,
+ };
+ struct nfs4_getattr_res res = {
+ .fattr = &fattr,
+ .server = server,
+ };
+ struct rpc_message msg = {
+ .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_GETATTR],
+ .rpc_argp = &args,
+ .rpc_resp = &res,
+ };
+ int ret;
+
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+ nfs_fattr_init(&fattr);
+
+ ret = rpc_call_sync(server->client, &msg, 0);
+ if (ret)
+ goto out;
+ if (!(fattr.bitmap[1] & FATTR4_WORD1_SECURITY_LABEL))
+ return -ENOENT;
+ if (buflen < fattr.label_len) {
+ ret = -ERANGE;
+ goto out;
+ }
+ memcpy(buf, fattr.label, fattr.label_len);
+out:
+ nfs_fattr_fini(&fattr);
+ return ret;
+}
+
+static int nfs4_get_security_label(struct inode *inode, void *buf, size_t buflen)
+{
+ struct nfs4_exception exception = { };
+ int err;
+
+ if (!nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL))
+ return -EOPNOTSUPP;
+
+ do {
+ err = nfs4_handle_exception(NFS_SERVER(inode),
+ _nfs4_get_security_label(inode, buf, buflen),
+ &exception);
+ } while (exception.retry);
+ return err;
+}
+
+static int _nfs4_do_set_security_label(struct inode *inode,
+ struct iattr *sattr,
+ struct nfs_fattr *fattr,
+ struct nfs4_state *state)
+{
+ struct nfs_server *server = NFS_SERVER(inode);
+ const u32 bitmask[2] = { 0, FATTR4_WORD1_SECURITY_LABEL };
+ struct nfs_setattrargs args = {
+ .fh = NFS_FH(inode),
+ .iap = sattr,
+ .server = server,
+ .bitmask = bitmask,
+ };
+ struct nfs_setattrres res = {
+ .fattr = fattr,
+ .server = server,
+ };
+ struct rpc_message msg = {
+ .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETATTR],
+ .rpc_argp = &args,
+ .rpc_resp = &res,
+ };
+ unsigned long timestamp = jiffies;
+ int status;
+
+ if (nfs4_copy_delegation_stateid(&args.stateid, inode)) {
+ /* Use that stateid */
+ } else if (state != NULL) {
+ msg.rpc_cred = state->owner->so_cred;
+ nfs4_copy_stateid(&args.stateid, state, current->files);
+ } else
+ memcpy(&args.stateid, &zero_stateid, sizeof(args.stateid));
+
+ status = rpc_call_sync(server->client, &msg, 0);
+ if (status == 0 && state != NULL)
+ renew_lease(server, timestamp);
+ return status;
+}
+
+static int nfs4_do_set_security_label(struct inode *inode,
+ struct iattr *sattr,
+ struct nfs_fattr *fattr,
+ struct nfs4_state *state)
+{
+ struct nfs4_exception exception = { };
+ int err;
+
+ do {
+ err = nfs4_handle_exception(NFS_SERVER(inode),
+ _nfs4_do_set_security_label(inode, sattr, fattr, state),
+ &exception);
+ } while (exception.retry);
+ return err;
+}
+
+static int
+nfs4_set_security_label(struct dentry *dentry, const void *buf, size_t buflen)
+{
+ struct nfs_fattr fattr;
+ struct iattr sattr;
+ struct rpc_cred *cred;
+ struct nfs_open_context *ctx;
+ struct nfs4_state *state = NULL;
+ struct inode *inode = dentry->d_inode;
+ int status;
+
+ if (!nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL))
+ return -EOPNOTSUPP;
+
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+ nfs_fattr_init(&fattr);
+
+ memset(&sattr, 0, sizeof(struct iattr));
+ sattr.ia_valid = ATTR_SECURITY_LABEL;
+ sattr.ia_label = (char *)buf;
+ sattr.ia_label_len = buflen;
+
+ cred = rpcauth_lookupcred(NFS_CLIENT(inode)->cl_auth, 0);
+ if (IS_ERR(cred))
+ return PTR_ERR(cred);
+
+ /* Search for an existing open(O_WRITE) file */
+ ctx = nfs_find_open_context(inode, cred, FMODE_WRITE);
+ if (ctx != NULL)
+ state = ctx->state;
+
+ status = nfs4_do_set_security_label(inode, &sattr, &fattr, state);
+ if (status == 0) {
+ nfs_setattr_update_inode(inode, &sattr);
+ fsnotify_change(dentry, sattr.ia_valid);
+ }
+ if (ctx != NULL)
+ put_nfs_open_context(ctx);
+ put_rpccred(cred);
+ nfs_fattr_fini(&fattr);
+ return status;
+}
+#endif /* CONFIG_NFS_V4_SECURITY_LABEL */
+
static int
nfs4_async_handle_error(struct rpc_task *task, const struct nfs_server *server)
{
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index db1ed9c..ed7ec8b 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -651,6 +651,10 @@ static int encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, const s
}
len += 4 + (XDR_QUADLEN(owner_grouplen) << 2);
}
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (iap->ia_valid & ATTR_SECURITY_LABEL)
+ len += 4 + (XDR_QUADLEN(iap->ia_label_len) << 2);
+#endif
if (iap->ia_valid & ATTR_ATIME_SET)
len += 16;
else if (iap->ia_valid & ATTR_ATIME)
@@ -709,6 +713,13 @@ static int encode_attrs(struct xdr_stream *xdr, const struct iattr *iap, const s
bmval1 |= FATTR4_WORD1_TIME_MODIFY_SET;
WRITE32(NFS4_SET_TO_SERVER_TIME);
}
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (iap->ia_valid & ATTR_SECURITY_LABEL) {
+ bmval1 |= FATTR4_WORD1_SECURITY_LABEL;
+ WRITE32(iap->ia_label_len);
+ WRITEMEM(iap->ia_label, iap->ia_label_len);
+ }
+#endif /* CONFIG_NFS_V4_SECURITY_LABEL */
/*
* Now we backfill the bitmap and the attribute buffer length.
@@ -2954,6 +2965,40 @@ static int decode_attr_time_modify(struct xdr_stream *xdr, uint32_t *bitmap, str
return status;
}
+static int decode_attr_security_label(struct xdr_stream *xdr, uint32_t *bitmap, char **ctx, u32 *ctxlen)
+{
+ uint32_t len;
+ __be32 *p;
+ int rc = 0;
+
+ if (unlikely(bitmap[1] & (FATTR4_WORD1_SECURITY_LABEL - 1U)))
+ return -EIO;
+ if (likely(bitmap[1] & FATTR4_WORD1_SECURITY_LABEL)) {
+ READ_BUF(4);
+ READ32(len);
+ READ_BUF(len);
+ if (len < XDR_MAX_NETOBJ) {
+ if (*ctx != NULL) {
+ if (*ctxlen < len) {
+ printk(KERN_ERR
+ "%s(): ctxlen (%d) < len (%d)\n",
+ __func__, *ctxlen, len);
+ /* rc = -ENOMEM; */
+ *ctx = NULL; /* leak */
+ } else {
+ memcpy(*ctx, (char *)p, len);
+ (*ctx)[len + 1] = '\0';
+ }
+ }
+ *ctxlen = len;
+ } else
+ printk(KERN_WARNING "%s: label too long (%u)!\n",
+ __FUNCTION__, len);
+ bitmap[1] &= ~FATTR4_WORD1_SECURITY_LABEL;
+ }
+ return rc;
+}
+
static int verify_attr_len(struct xdr_stream *xdr, __be32 *savep, uint32_t attrlen)
{
unsigned int attrwords = XDR_QUADLEN(attrlen);
@@ -3186,6 +3231,10 @@ static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr, cons
goto xdr_error;
if ((status = decode_attr_mounted_on_fileid(xdr, bitmap, &fileid)) != 0)
goto xdr_error;
+ if ((status = decode_attr_security_label(xdr, bitmap,
+ &fattr->label,
+ &fattr->label_len)) != 0)
+ goto xdr_error;
if (fattr->fileid == 0 && fileid != 0)
fattr->fileid = fileid;
if ((status = verify_attr_len(xdr, savep, attrlen)) == 0)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 95f2fad..815ca3c 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -492,6 +492,13 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss,
seq_printf(m, ",timeo=%lu", 10U * nfss->client->cl_timeout->to_initval / HZ);
seq_printf(m, ",retrans=%u", nfss->client->cl_timeout->to_retries);
seq_printf(m, ",sec=%s", nfs_pseudoflavour_to_name(nfss->client->cl_auth->au_flavor));
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if ((nfss->nfs_client->rpc_ops->version == 4) &&
+ (nfss->attr_bitmask[1] & FATTR4_WORD1_SECURITY_LABEL))
+ seq_printf(m, ",security_label");
+#endif /* CONFIG_NFS_V4_SECURITY_LABEL */
+
}
/*
@@ -547,6 +554,10 @@ static int nfs_show_stats(struct seq_file *m, struct vfsmount *mnt)
seq_printf(m, "bm0=0x%x", nfss->attr_bitmask[0]);
seq_printf(m, ",bm1=0x%x", nfss->attr_bitmask[1]);
seq_printf(m, ",acl=0x%x", nfss->acl_bitmask);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfss->attr_bitmask[1] & FATTR4_WORD1_SECURITY_LABEL)
+ seq_printf(m, ",security_label");
+#endif /* CONFIG_NFS_V4_SECURITY_LABEL */
}
#endif
diff --git a/security/security.c b/security/security.c
index b6e80bb..1276c98 100644
--- a/security/security.c
+++ b/security/security.c
@@ -517,6 +517,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
return 0;
return security_ops->inode_setsecurity(inode, name, value, size, flags);
}
+EXPORT_SYMBOL(security_inode_setsecurity);
int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
{
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e3ed7c3..1742aaa 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2683,7 +2683,11 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, char *name,
return;
}
+ isec->sclass = inode_mode_to_security_class(inode->i_mode);
isec->sid = newsid;
+ isec->initialized = 1;
+
+ fsnotify_change(dentry, ATTR_SECURITY_LABEL);
return;
}
@@ -2754,7 +2758,9 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
if (rc)
return rc;
- isec->sid = newsid;
+ isec->sclass = inode_mode_to_security_class(inode->i_mode);
+ isec->sid = newsid;
+ isec->initialized = 1;
return 0;
}
--
1.5.3.8
Two fields have been added to the nfs_fattr structure to carry the security
label and its length. This has raised the need to provide lifecycle management
for these values. This patch introduces two macros nfs_fattr_alloc and
nfs_fattr_fini which are used to allocate and destroy these fields inside the
nfs_fattr structure. These macros do not modify any other components of the
structure so nfs_fattr_init still has to be used on these structures. In the
event that CONFIG_SECURITY is not set these calls should compile away.
Signed-off-by: David P. Quigley <[email protected]>
---
fs/nfs/client.c | 16 ++++++
fs/nfs/dir.c | 25 ++++++++++
fs/nfs/getroot.c | 33 +++++++++++++
fs/nfs/inode.c | 16 ++++++
fs/nfs/namespace.c | 3 +
fs/nfs/nfs3proc.c | 15 ++++++
fs/nfs/nfs4proc.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++-
fs/nfs/proc.c | 13 +++++-
fs/nfs/super.c | 4 ++
include/linux/nfs_fs.h | 41 ++++++++++++++++
10 files changed, 283 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index c5c0175..aa93f9d 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -878,6 +878,8 @@ struct nfs_server *nfs_create_server(const struct nfs_parsed_mount_data *data,
struct nfs_fattr fattr;
int error;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
server = nfs_alloc_server();
if (!server)
return ERR_PTR(-ENOMEM);
@@ -928,10 +930,12 @@ struct nfs_server *nfs_create_server(const struct nfs_parsed_mount_data *data,
spin_unlock(&nfs_client_lock);
server->mount_time = jiffies;
+ nfs_fattr_fini(&fattr);
return server;
error:
nfs_free_server(server);
+ nfs_fattr_fini(&fattr);
return ERR_PTR(error);
}
@@ -1083,6 +1087,8 @@ struct nfs_server *nfs4_create_server(const struct nfs_parsed_mount_data *data,
dprintk("--> nfs4_create_server()\n");
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
server = nfs_alloc_server();
if (!server)
return ERR_PTR(-ENOMEM);
@@ -1123,11 +1129,13 @@ struct nfs_server *nfs4_create_server(const struct nfs_parsed_mount_data *data,
spin_unlock(&nfs_client_lock);
server->mount_time = jiffies;
+ nfs_fattr_fini(&fattr);
dprintk("<-- nfs4_create_server() = %p\n", server);
return server;
error:
nfs_free_server(server);
+ nfs_fattr_fini(&fattr);
dprintk("<-- nfs4_create_server() = error %d\n", error);
return ERR_PTR(error);
}
@@ -1145,6 +1153,8 @@ struct nfs_server *nfs4_create_referral_server(struct nfs_clone_mount *data,
dprintk("--> nfs4_create_referral_server()\n");
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
server = nfs_alloc_server();
if (!server)
return ERR_PTR(-ENOMEM);
@@ -1200,11 +1210,13 @@ struct nfs_server *nfs4_create_referral_server(struct nfs_clone_mount *data,
server->mount_time = jiffies;
+ nfs_fattr_fini(&fattr);
dprintk("<-- nfs_create_referral_server() = %p\n", server);
return server;
error:
nfs_free_server(server);
+ nfs_fattr_fini(&fattr);
dprintk("<-- nfs4_create_referral_server() = error %d\n", error);
return ERR_PTR(error);
}
@@ -1226,6 +1238,8 @@ struct nfs_server *nfs_clone_server(struct nfs_server *source,
(unsigned long long) fattr->fsid.major,
(unsigned long long) fattr->fsid.minor);
+ memset(&fattr_fsinfo, 0, sizeof(struct nfs_fattr));
+
server = nfs_alloc_server();
if (!server)
return ERR_PTR(-ENOMEM);
@@ -1268,11 +1282,13 @@ struct nfs_server *nfs_clone_server(struct nfs_server *source,
server->mount_time = jiffies;
+ nfs_fattr_fini(&fattr_fsinfo);
dprintk("<-- nfs_clone_server() = %p\n", server);
return server;
out_free_server:
nfs_free_server(server);
+ nfs_fattr_fini(&fattr_fsinfo);
dprintk("<-- nfs_clone_server() = error %d\n", error);
return ERR_PTR(error);
}
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index ae04892..19808be 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -533,6 +533,8 @@ static int nfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
(long long)filp->f_pos);
nfs_inc_stats(inode, NFSIOS_VFSGETDENTS);
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
lock_kernel();
/*
@@ -589,6 +591,7 @@ static int nfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
res = 0;
break;
}
+ nfs_fattr_fini(&fattr);
}
out:
nfs_unblock_sillyrename(dentry);
@@ -764,6 +767,8 @@ static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd)
struct nfs_fh fhandle;
struct nfs_fattr fattr;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
parent = dget_parent(dentry);
lock_kernel();
dir = parent->d_inode;
@@ -793,6 +798,11 @@ static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd)
if (NFS_STALE(inode))
goto out_bad;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL))
+ nfs_fattr_alloc(&fattr, GFP_NOWAIT);
+#endif
+
error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle, &fattr);
if (error)
goto out_bad;
@@ -805,6 +815,7 @@ static int nfs_lookup_revalidate(struct dentry * dentry, struct nameidata *nd)
out_valid:
unlock_kernel();
dput(parent);
+ nfs_fattr_fini(&fattr);
dfprintk(LOOKUPCACHE, "NFS: %s(%s/%s) is valid\n",
__FUNCTION__, dentry->d_parent->d_name.name,
dentry->d_name.name);
@@ -824,6 +835,7 @@ out_zap_parent:
d_drop(dentry);
unlock_kernel();
dput(parent);
+ nfs_fattr_fini(&fattr);
dfprintk(LOOKUPCACHE, "NFS: %s(%s/%s) is invalid\n",
__FUNCTION__, dentry->d_parent->d_name.name,
dentry->d_name.name);
@@ -894,6 +906,8 @@ static struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, stru
dentry->d_parent->d_name.name, dentry->d_name.name);
nfs_inc_stats(dir, NFSIOS_VFSLOOKUP);
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
res = ERR_PTR(-ENAMETOOLONG);
if (dentry->d_name.len > NFS_SERVER(dir)->namelen)
goto out;
@@ -915,6 +929,11 @@ static struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, stru
parent = dentry->d_parent;
/* Protect against concurrent sillydeletes */
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL))
+ nfs_fattr_alloc(&fattr, GFP_NOWAIT);
+#endif
+
nfs_block_sillyrename(parent);
error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle, &fattr);
if (error == -ENOENT)
@@ -941,6 +960,8 @@ out_unblock_sillyrename:
out_unlock:
unlock_kernel();
out:
+ /* Label will give 'unused' warning on 'no_entry' case. */
+ nfs_fattr_fini(&fattr);
return res;
}
@@ -1209,6 +1230,7 @@ static int nfs_create(struct inode *dir, struct dentry *dentry, int mode,
dfprintk(VFS, "NFS: create(%s/%ld), %s\n",
dir->i_sb->s_id, dir->i_ino, dentry->d_name.name);
+ memset(&attr, 0, sizeof(struct iattr));
attr.ia_mode = mode;
attr.ia_valid = ATTR_MODE;
@@ -1242,6 +1264,7 @@ nfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
if (!new_valid_dev(rdev))
return -EINVAL;
+ memset(&attr, 0, sizeof(struct iattr));
attr.ia_mode = mode;
attr.ia_valid = ATTR_MODE;
@@ -1268,6 +1291,7 @@ static int nfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
dfprintk(VFS, "NFS: mkdir(%s/%ld), %s\n",
dir->i_sb->s_id, dir->i_ino, dentry->d_name.name);
+ memset(&attr, 0, sizeof(struct iattr));
attr.ia_valid = ATTR_MODE;
attr.ia_mode = mode | S_IFDIR;
@@ -1485,6 +1509,7 @@ static int nfs_symlink(struct inode *dir, struct dentry *dentry, const char *sym
if (pathlen > PAGE_SIZE)
return -ENAMETOOLONG;
+ memset(&attr, 0, sizeof(struct iattr));
attr.ia_mode = S_IFLNK | S_IRWXUGO;
attr.ia_valid = ATTR_MODE;
diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
index fae9719..d0ed440 100644
--- a/fs/nfs/getroot.c
+++ b/fs/nfs/getroot.c
@@ -84,6 +84,8 @@ struct dentry *nfs_get_root(struct super_block *sb, struct nfs_fh *mntfh)
struct inode *inode;
int error;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
/* get the actual root for this mount */
fsinfo.fattr = &fattr;
@@ -119,6 +121,7 @@ struct dentry *nfs_get_root(struct super_block *sb, struct nfs_fh *mntfh)
if (!mntroot->d_op)
mntroot->d_op = server->nfs_client->rpc_ops->dentry_ops;
+ nfs_fattr_fini(&fattr);
return mntroot;
}
@@ -143,6 +146,11 @@ int nfs4_path_walk(struct nfs_server *server,
dprintk("--> nfs4_path_walk(,,%s)\n", path);
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+#endif
fsinfo.fattr = &fattr;
nfs_fattr_init(&fattr);
@@ -154,12 +162,14 @@ int nfs4_path_walk(struct nfs_server *server,
ret = server->nfs_client->rpc_ops->getroot(server, mntfh, &fsinfo);
if (ret < 0) {
dprintk("nfs4_get_root: getroot error = %d\n", -ret);
+ nfs_fattr_fini(&fattr);
return ret;
}
if (fattr.type != NFDIR) {
printk(KERN_ERR "nfs4_get_root:"
" getroot encountered non-directory\n");
+ nfs_fattr_fini(&fattr);
return -ENOTDIR;
}
@@ -167,6 +177,7 @@ int nfs4_path_walk(struct nfs_server *server,
if (fattr.valid & NFS_ATTR_FATTR_V4_REFERRAL) {
printk(KERN_ERR "nfs4_get_root:"
" getroot obtained referral\n");
+ nfs_fattr_fini(&fattr);
return -EREMOTE;
}
@@ -199,6 +210,7 @@ eat_dot_dir:
) {
printk(KERN_ERR "nfs4_get_root:"
" Mount path contains reference to \"..\"\n");
+ nfs_fattr_fini(&fattr);
return -EINVAL;
}
@@ -207,16 +219,25 @@ eat_dot_dir:
dprintk("LookupFH: %*.*s [%s]\n", name.len, name.len, name.name, path);
+ nfs_fattr_fini(&fattr);
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+#endif
+
ret = server->nfs_client->rpc_ops->lookupfh(server, &lastfh, &name,
mntfh, &fattr);
if (ret < 0) {
dprintk("nfs4_get_root: getroot error = %d\n", -ret);
+ nfs_fattr_fini(&fattr);
return ret;
}
if (fattr.type != NFDIR) {
printk(KERN_ERR "nfs4_get_root:"
" lookupfh encountered non-directory\n");
+ nfs_fattr_fini(&fattr);
return -ENOTDIR;
}
@@ -224,6 +245,7 @@ eat_dot_dir:
if (fattr.valid & NFS_ATTR_FATTR_V4_REFERRAL) {
printk(KERN_ERR "nfs4_get_root:"
" lookupfh obtained referral\n");
+ nfs_fattr_fini(&fattr);
return -EREMOTE;
}
@@ -231,6 +253,7 @@ eat_dot_dir:
path_walk_complete:
memcpy(&server->fsid, &fattr.fsid, sizeof(server->fsid));
+ nfs_fattr_fini(&fattr);
dprintk("<-- nfs4_path_walk() = 0\n");
return 0;
}
@@ -256,18 +279,28 @@ struct dentry *nfs4_get_root(struct super_block *sb, struct nfs_fh *mntfh)
return ERR_PTR(error);
}
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+#endif
+
/* get the actual root for this mount */
error = server->nfs_client->rpc_ops->getattr(server, mntfh, &fattr);
if (error < 0) {
dprintk("nfs_get_root: getattr error = %d\n", -error);
+ nfs_fattr_fini(&fattr);
return ERR_PTR(error);
}
inode = nfs_fhget(sb, mntfh, &fattr);
if (IS_ERR(inode)) {
dprintk("nfs_get_root: get root inode failed\n");
+ nfs_fattr_fini(&fattr);
return ERR_CAST(inode);
}
+
+ nfs_fattr_fini(&fattr);
error = nfs_superblock_set_dummy_root(sb, inode);
if (error != 0)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 966a885..c34fb7c 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -357,6 +357,12 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
nfs_inc_stats(inode, NFSIOS_VFSSETATTR);
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL))
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+#endif
+
/* skip mode change if it's just for clearing setuid/setgid */
if (attr->ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
attr->ia_valid &= ~ATTR_MODE;
@@ -386,6 +392,7 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
if (error == 0)
nfs_refresh_inode(inode, &fattr);
unlock_kernel();
+ nfs_fattr_fini(&fattr);
return error;
}
@@ -642,6 +649,9 @@ __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
inode->i_sb->s_id, (long long)NFS_FILEID(inode));
nfs_inc_stats(inode, NFSIOS_INODEREVALIDATE);
+
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
lock_kernel();
if (is_bad_inode(inode))
goto out_nowait;
@@ -656,6 +666,11 @@ __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
if (NFS_STALE(inode))
goto out;
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL))
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+#endif
+
status = NFS_PROTO(inode)->getattr(server, NFS_FH(inode), &fattr);
if (status != 0) {
dfprintk(PAGECACHE, "nfs_revalidate_inode: (%s/%Ld) getattr failed, error=%d\n",
@@ -692,6 +707,7 @@ __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
out_nowait:
unlock_kernel();
+ nfs_fattr_fini(&fattr);
return status;
}
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 607f6eb..b0a9791 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -105,6 +105,8 @@ static void * nfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
dprintk("--> nfs_follow_mountpoint()\n");
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
BUG_ON(IS_ROOT(dentry));
dprintk("%s: enter\n", __FUNCTION__);
dput(nd->path.dentry);
@@ -143,6 +145,7 @@ static void * nfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
nd->path.dentry = dget(mnt->mnt_root);
schedule_delayed_work(&nfs_automount_task, nfs_mountpoint_expiry_timeout);
out:
+ nfs_fattr_fini(&fattr);
dprintk("%s: done, returned %d\n", __FUNCTION__, err);
dprintk("<-- nfs_follow_mountpoint() = %d\n", err);
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 549dbce..2204507 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -380,6 +380,7 @@ nfs3_proc_unlink_done(struct rpc_task *task, struct inode *dir)
return 0;
res = task->tk_msg.rpc_resp;
nfs_post_op_update_inode(dir, &res->dir_attr);
+ nfs_fattr_fini(&res->dir_attr);
return 1;
}
@@ -479,6 +480,9 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page,
dprintk("NFS call symlink %s\n", dentry->d_name.name);
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ memset(&dir_attr, 0, sizeof(struct nfs_fattr));
+
nfs_fattr_init(&dir_attr);
nfs_fattr_init(&fattr);
status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
@@ -487,6 +491,8 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page,
goto out;
status = nfs_instantiate(dentry, &fhandle, &fattr);
out:
+ nfs_fattr_fini(&fattr);
+ nfs_fattr_fini(&dir_attr);
dprintk("NFS reply symlink: %d\n", status);
return status;
}
@@ -519,6 +525,8 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
sattr->ia_mode &= ~current->fs->umask;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ memset(&dir_attr, 0, sizeof(struct nfs_fattr));
nfs_fattr_init(&dir_attr);
nfs_fattr_init(&fattr);
status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
@@ -530,6 +538,8 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
goto out;
status = nfs3_proc_set_default_acl(dir, dentry->d_inode, mode);
out:
+ nfs_fattr_fini(&fattr);
+ nfs_fattr_fini(&dir_attr);
dprintk("NFS reply mkdir: %d\n", status);
return status;
}
@@ -637,6 +647,9 @@ nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
mode_t mode = sattr->ia_mode;
int status;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ memset(&dir_attr, 0, sizeof(struct nfs_fattr));
+
switch (sattr->ia_mode & S_IFMT) {
case S_IFBLK: arg.type = NF3BLK; break;
case S_IFCHR: arg.type = NF3CHR; break;
@@ -661,6 +674,8 @@ nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
goto out;
status = nfs3_proc_set_default_acl(dir, dentry->d_inode, mode);
out:
+ nfs_fattr_fini(&fattr);
+ nfs_fattr_fini(&dir_attr);
dprintk("NFS reply mknod: %d\n", status);
return status;
}
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 42e5cf7..b278f7c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -243,6 +243,8 @@ static void nfs4_init_opendata_res(struct nfs4_opendata *p)
p->o_res.f_attr = &p->f_attr;
p->o_res.dir_attr = &p->dir_attr;
p->o_res.server = p->o_arg.server;
+ memset(&p->f_attr, 0, sizeof(struct nfs_fattr));
+ memset(&p->dir_attr, 0, sizeof(struct nfs_fattr));
nfs_fattr_init(&p->f_attr);
nfs_fattr_init(&p->dir_attr);
}
@@ -275,6 +277,7 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
p->o_arg.server = server;
p->o_arg.bitmask = server->attr_bitmask;
p->o_arg.claim = NFS4_OPEN_CLAIM_NULL;
+ memset(&p->attrs, 0, sizeof(struct iattr));
if (flags & O_EXCL) {
u32 *s = (u32 *) p->o_arg.u.verifier.data;
s[0] = jiffies;
@@ -282,12 +285,22 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
} else if (flags & O_CREAT) {
p->o_arg.u.attrs = &p->attrs;
memcpy(&p->attrs, attrs, sizeof(p->attrs));
+ /* The above creates an additional reference to ia_label.
+ * The CALLER must free this, not nfs4_opendata_free()
+ */
}
p->c_arg.fh = &p->o_res.fh;
p->c_arg.stateid = &p->o_res.stateid;
p->c_arg.seqid = p->o_arg.seqid;
nfs4_init_opendata_res(p);
kref_init(&p->kref);
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL) {
+ nfs_fattr_alloc(&p->f_attr, GFP_KERNEL);
+ nfs_fattr_alloc(&p->dir_attr, GFP_KERNEL);
+ }
+#endif
return p;
err_free:
kfree(p);
@@ -304,6 +317,8 @@ static void nfs4_opendata_free(struct kref *kref)
nfs_free_seqid(p->o_arg.seqid);
if (p->state != NULL)
nfs4_put_open_state(p->state);
+ nfs_fattr_fini(&p->f_attr);
+ nfs_fattr_fini(&p->dir_attr);
nfs4_put_state_owner(p->owner);
dput(p->dir);
dput(p->path.dentry);
@@ -1215,6 +1230,7 @@ static void nfs4_free_closedata(void *data)
nfs4_put_state_owner(sp);
dput(calldata->path.dentry);
mntput(calldata->path.mnt);
+ nfs_fattr_fini(&calldata->fattr);
kfree(calldata);
}
@@ -1322,7 +1338,7 @@ int nfs4_do_close(struct path *path, struct nfs4_state *state, int wait)
};
int status = -ENOMEM;
- calldata = kmalloc(sizeof(*calldata), GFP_KERNEL);
+ calldata = kzalloc(sizeof(*calldata), GFP_KERNEL);
if (calldata == NULL)
goto out;
calldata->inode = state->inode;
@@ -1339,6 +1355,11 @@ int nfs4_do_close(struct path *path, struct nfs4_state *state, int wait)
calldata->path.mnt = mntget(path->mnt);
calldata->path.dentry = dget(path->dentry);
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(&calldata->fattr, GFP_KERNEL);
+#endif
+
msg.rpc_argp = &calldata->arg,
msg.rpc_resp = &calldata->res,
task_setup_data.callback_data = calldata;
@@ -1351,6 +1372,7 @@ int nfs4_do_close(struct path *path, struct nfs4_state *state, int wait)
rpc_put_task(task);
return status;
out_free_calldata:
+ nfs_fattr_fini(&calldata->fattr);
kfree(calldata);
out:
nfs4_put_open_state(state);
@@ -1397,6 +1419,7 @@ nfs4_atomic_open(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
struct nfs4_state *state;
struct dentry *res;
+ memset(&attr, 0, sizeof(struct iattr));
if (nd->flags & LOOKUP_CREATE) {
attr.ia_mode = nd->intent.open.create_mode;
attr.ia_valid = ATTR_MODE;
@@ -1770,6 +1793,8 @@ static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry
int mode = entry->mask;
int status;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
/*
* Determine which access bits we want to ask for...
*/
@@ -1786,6 +1811,10 @@ static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry
if (mode & MAY_EXEC)
args.access |= NFS4_ACCESS_EXECUTE;
}
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+#endif
nfs_fattr_init(&fattr);
status = rpc_call_sync(NFS_CLIENT(inode), &msg, 0);
if (!status) {
@@ -1798,6 +1827,7 @@ static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry
entry->mask |= MAY_EXEC;
nfs_refresh_inode(inode, &fattr);
}
+ nfs_fattr_fini(&fattr);
return status;
}
@@ -1911,10 +1941,17 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
if (flags & O_EXCL) {
struct nfs_fattr fattr;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (nfs_server_capable(state->inode, NFS_CAP_SECURITY_LABEL))
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+#endif
status = nfs4_do_setattr(state->inode, &fattr, sattr, state);
if (status == 0)
nfs_setattr_update_inode(state->inode, sattr);
nfs_post_op_update_inode(state->inode, &fattr);
+
+ nfs_fattr_fini(&fattr);
}
if (status == 0 && (nd->flags & LOOKUP_OPEN) != 0)
status = nfs4_intent_set_file(nd, &path, state);
@@ -1943,12 +1980,18 @@ static int _nfs4_proc_remove(struct inode *dir, struct qstr *name)
};
int status;
+ memset(&res.dir_attr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(&res.dir_attr, GFP_KERNEL);
+#endif
nfs_fattr_init(&res.dir_attr);
status = rpc_call_sync(server->client, &msg, 0);
if (status == 0) {
update_changeattr(dir, &res.cinfo);
nfs_post_op_update_inode(dir, &res.dir_attr);
}
+ nfs_fattr_fini(&res.dir_attr);
return status;
}
@@ -1973,6 +2016,13 @@ static void nfs4_proc_unlink_setup(struct rpc_message *msg, struct inode *dir)
args->bitmask = server->attr_bitmask;
res->server = server;
msg->rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_REMOVE];
+
+ memset(&res->dir_attr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(&res->dir_attr, GFP_KERNEL);
+#endif
+ nfs_fattr_init(&res->dir_attr);
}
static int nfs4_proc_unlink_done(struct rpc_task *task, struct inode *dir)
@@ -1983,6 +2033,7 @@ static int nfs4_proc_unlink_done(struct rpc_task *task, struct inode *dir)
return 0;
update_changeattr(dir, &res->cinfo);
nfs_post_op_update_inode(dir, &res->dir_attr);
+ nfs_fattr_fini(&res->dir_attr);
return 1;
}
@@ -2010,6 +2061,15 @@ static int _nfs4_proc_rename(struct inode *old_dir, struct qstr *old_name,
};
int status;
+
+ memset(&old_fattr, 0, sizeof(struct nfs_fattr));
+ memset(&new_fattr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL) {
+ nfs_fattr_alloc(&old_fattr, GFP_KERNEL);
+ nfs_fattr_alloc(&new_fattr, GFP_KERNEL);
+ }
+#endif
nfs_fattr_init(res.old_fattr);
nfs_fattr_init(res.new_fattr);
status = rpc_call_sync(server->client, &msg, 0);
@@ -2020,6 +2080,8 @@ static int _nfs4_proc_rename(struct inode *old_dir, struct qstr *old_name,
update_changeattr(new_dir, &res.new_cinfo);
nfs_post_op_update_inode(new_dir, res.new_fattr);
}
+ nfs_fattr_fini(&old_fattr);
+ nfs_fattr_fini(&new_fattr);
return status;
}
@@ -2059,6 +2121,16 @@ static int _nfs4_proc_link(struct inode *inode, struct inode *dir, struct qstr *
};
int status;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ memset(&dir_attr, 0, sizeof(struct nfs_fattr));
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL) {
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+ nfs_fattr_alloc(&dir_attr, GFP_KERNEL);
+ }
+#endif
+
nfs_fattr_init(res.fattr);
nfs_fattr_init(res.dir_attr);
status = rpc_call_sync(server->client, &msg, 0);
@@ -2068,6 +2140,8 @@ static int _nfs4_proc_link(struct inode *inode, struct inode *dir, struct qstr *
nfs_post_op_update_inode(inode, res.fattr);
}
+ nfs_fattr_fini(&fattr);
+ nfs_fattr_fini(&dir_attr);
return status;
}
@@ -2113,6 +2187,16 @@ static int _nfs4_proc_symlink(struct inode *dir, struct dentry *dentry,
if (len > NFS4_MAXPATHLEN)
return -ENAMETOOLONG;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ memset(&dir_fattr, 0, sizeof(struct nfs_fattr));
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL) {
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+ nfs_fattr_alloc(&dir_fattr, GFP_KERNEL);
+ }
+#endif
+
arg.u.symlink.pages = &page;
arg.u.symlink.len = len;
nfs_fattr_init(&fattr);
@@ -2124,6 +2208,8 @@ static int _nfs4_proc_symlink(struct inode *dir, struct dentry *dentry,
nfs_post_op_update_inode(dir, res.dir_fattr);
status = nfs_instantiate(dentry, &fhandle, &fattr);
}
+ nfs_fattr_fini(&fattr);
+ nfs_fattr_fini(&dir_fattr);
return status;
}
@@ -2168,6 +2254,16 @@ static int _nfs4_proc_mkdir(struct inode *dir, struct dentry *dentry,
};
int status;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ memset(&dir_fattr, 0, sizeof(struct nfs_fattr));
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL) {
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+ nfs_fattr_alloc(&dir_fattr, GFP_KERNEL);
+ }
+#endif
+
nfs_fattr_init(&fattr);
nfs_fattr_init(&dir_fattr);
@@ -2177,6 +2273,8 @@ static int _nfs4_proc_mkdir(struct inode *dir, struct dentry *dentry,
nfs_post_op_update_inode(dir, res.dir_fattr);
status = nfs_instantiate(dentry, &fhandle, &fattr);
}
+ nfs_fattr_fini(&fattr);
+ nfs_fattr_fini(&dir_fattr);
return status;
}
@@ -2270,6 +2368,15 @@ static int _nfs4_proc_mknod(struct inode *dir, struct dentry *dentry,
int status;
int mode = sattr->ia_mode;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ memset(&dir_fattr, 0, sizeof(struct nfs_fattr));
+
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (server->caps & NFS_CAP_SECURITY_LABEL) {
+ nfs_fattr_alloc(&fattr, GFP_KERNEL);
+ nfs_fattr_alloc(&dir_fattr, GFP_KERNEL);
+ }
+#endif
nfs_fattr_init(&fattr);
nfs_fattr_init(&dir_fattr);
@@ -2296,6 +2403,8 @@ static int _nfs4_proc_mknod(struct inode *dir, struct dentry *dentry,
nfs_post_op_update_inode(dir, res.dir_fattr);
status = nfs_instantiate(dentry, &fh, &fattr);
}
+ nfs_fattr_fini(&fattr);
+ nfs_fattr_fini(&dir_fattr);
return status;
}
@@ -2973,6 +3082,9 @@ static void nfs4_delegreturn_done(struct rpc_task *task, void *calldata)
static void nfs4_delegreturn_release(void *calldata)
{
+ struct nfs4_delegreturndata *data = calldata;
+
+ nfs_fattr_fini(data->res.fattr);
kfree(calldata);
}
@@ -3008,6 +3120,11 @@ static int _nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, co
memcpy(&data->stateid, stateid, sizeof(data->stateid));
data->res.fattr = &data->fattr;
data->res.server = server;
+ memset(data->res.fattr, 0, sizeof(struct nfs_fattr));
+#ifdef CONFIG_NFS_V4_SECURITY_LABEL
+ if (data->res.server->caps & NFS_CAP_SECURITY_LABEL)
+ nfs_fattr_alloc(data->res.fattr, GFP_KERNEL);
+#endif
nfs_fattr_init(data->res.fattr);
data->timestamp = jiffies;
data->rpc_status = 0;
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index 5ccf7fa..d7435f6 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -208,12 +208,15 @@ nfs_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
};
int status;
- nfs_fattr_init(&fattr);
dprintk("NFS call create %s\n", dentry->d_name.name);
+
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+ nfs_fattr_init(&fattr);
status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
nfs_mark_for_revalidate(dir);
if (status == 0)
status = nfs_instantiate(dentry, &fhandle, &fattr);
+ nfs_fattr_fini(&fattr);
dprintk("NFS reply create: %d\n", status);
return status;
}
@@ -255,6 +258,7 @@ nfs_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
sattr->ia_size = new_encode_dev(rdev);/* get out your barf bag */
}
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
nfs_fattr_init(&fattr);
status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
nfs_mark_for_revalidate(dir);
@@ -266,6 +270,7 @@ nfs_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
}
if (status == 0)
status = nfs_instantiate(dentry, &fhandle, &fattr);
+ nfs_fattr_fini(&fattr);
dprintk("NFS reply mknod: %d\n", status);
return status;
}
@@ -378,6 +383,8 @@ nfs_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page,
dprintk("NFS call symlink %s\n", dentry->d_name.name);
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
nfs_mark_for_revalidate(dir);
@@ -392,6 +399,7 @@ nfs_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page,
status = nfs_instantiate(dentry, &fhandle, &fattr);
}
+ nfs_fattr_fini(&fattr);
dprintk("NFS reply symlink: %d\n", status);
return status;
}
@@ -419,11 +427,14 @@ nfs_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
int status;
dprintk("NFS call mkdir %s\n", dentry->d_name.name);
+
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
nfs_fattr_init(&fattr);
status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
nfs_mark_for_revalidate(dir);
if (status == 0)
status = nfs_instantiate(dentry, &fhandle, &fattr);
+ nfs_fattr_fini(&fattr);
dprintk("NFS reply mkdir: %d\n", status);
return status;
}
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index f3e327e..95f2fad 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -371,6 +371,8 @@ static int nfs_statfs(struct dentry *dentry, struct kstatfs *buf)
};
int error;
+ memset(&fattr, 0, sizeof(struct nfs_fattr));
+
lock_kernel();
error = server->nfs_client->rpc_ops->statfs(server, fh, &res);
@@ -405,11 +407,13 @@ static int nfs_statfs(struct dentry *dentry, struct kstatfs *buf)
buf->f_namelen = server->namelen;
unlock_kernel();
+ nfs_fattr_fini(&fattr);
return 0;
out_err:
dprintk("%s: statfs error = %d\n", __FUNCTION__, -error);
unlock_kernel();
+ nfs_fattr_fini(&fattr);
return error;
}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index a69ba80..991d56f 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -345,6 +345,47 @@ static inline void nfs_fattr_init(struct nfs_fattr *fattr)
fattr->time_start = jiffies;
}
+#ifdef CONFIG_SECURITY
+static inline void nfs_fattr_alloc(struct nfs_fattr *fattr, gfp_t flags)
+{
+ fattr->label = kzalloc(NFS4_MAXLABELLEN, flags);
+ if (fattr->label == NULL)
+ panic("Can't allocate security label.");
+ fattr->label_len = NFS4_MAXLABELLEN;
+}
+
+#define nfs_fattr_fini(fattr) _nfs_fattr_fini(fattr, __FILE__, __LINE__, __func__)
+static inline void _nfs_fattr_fini(struct nfs_fattr *fattr,
+ const char *file, int line, const char *func)
+{
+ if ((fattr)->label == NULL) {
+ if (fattr->label_len != 0) {
+ printk(KERN_WARNING
+ "%s:%d %s() nfs_fattr label available (%d)\n",
+ file, line, func,
+ fattr->label_len);
+ }
+ } else {
+ if (fattr->label_len == NFS4_MAXLABELLEN)
+ printk(KERN_WARNING
+ "%s:%d %s() nfs_fattr label unused\n",
+ file, line, func);
+ else if (fattr->label_len != (strlen(fattr->label) + 1))
+ printk(KERN_WARNING
+ "%s:%d %s() nfs_fattr label size mismatch (label_len %d, strlen %d)\n",
+ file, line, func,
+ fattr->label_len, strlen(fattr->label) + 1);
+
+ kfree(fattr->label);
+ fattr->label = NULL;
+ fattr->label_len = 0;
+ }
+}
+#else
+#define nfs_fattr_alloc(fattr, flags)
+#define nfs_fattr_fini(fattr)
+#endif
+
/*
* linux/fs/nfs/file.c
*/
--
1.5.3.8
On Wed, 27 Feb 2008, David P. Quigley wrote:
> +#ifdef CONFIG_SECURITY
> +static inline void nfs_fattr_alloc(struct nfs_fattr *fattr, gfp_t flags)
> +{
> + fattr->label = kzalloc(NFS4_MAXLABELLEN, flags);
> + if (fattr->label == NULL)
> + panic("Can't allocate security label.");
> + fattr->label_len = NFS4_MAXLABELLEN;
> +}
A panic here seems like overkill, and also possibly a DoS vector. I
suggest having the calling code handle the allocation failure gracefully.
> +
> +#define nfs_fattr_fini(fattr) _nfs_fattr_fini(fattr, __FILE__, __LINE__, __func__)
> +static inline void _nfs_fattr_fini(struct nfs_fattr *fattr,
> + const char *file, int line, const char *func)
> +{
> + if ((fattr)->label == NULL) {
> + if (fattr->label_len != 0) {
> + printk(KERN_WARNING
> + "%s:%d %s() nfs_fattr label available (%d)\n",
> + file, line, func,
> + fattr->label_len);
> + }
> + } else {
> + if (fattr->label_len == NFS4_MAXLABELLEN)
> + printk(KERN_WARNING
> + "%s:%d %s() nfs_fattr label unused\n",
> + file, line, func);
> + else if (fattr->label_len != (strlen(fattr->label) + 1))
> + printk(KERN_WARNING
> + "%s:%d %s() nfs_fattr label size mismatch (label_len %d, strlen %d)\n",
> + file, line, func,
> + fattr->label_len, strlen(fattr->label) + 1);
> +
> + kfree(fattr->label);
> + fattr->label = NULL;
> + fattr->label_len = 0;
> + }
> +}
> +#else
> +#define nfs_fattr_alloc(fattr, flags)
> +#define nfs_fattr_fini(fattr)
> +#endif
Perhaps introduce a debug configuration option for this code.
- James
--
James Morris
<[email protected]>
--- Dave Quigley <[email protected]> wrote:
>
> On Wed, 2008-02-27 at 15:42 -0800, Casey Schaufler wrote:
> > --- "David P. Quigley" <[email protected]> wrote:
> >
> > ...
> > > + const char *(*maclabel_getname) (void);
> >
> > I think that calling this a maclabel is a really bad idea. For one
> > thing, it assumes that all interesting security attributes are for
> > Mandatory Access Control. Also, it assumes that they are stored as
> > xattrs. While these conditions are both met by the two current LSMs
> > I would suggest that this is not a fair assumption for the long
> > haul unless the intention is to lock the lSM into only supporting
> > xattr based label based MAC modules.
>
> Actually that is a completely fair assumption.
A completely reasonable LSM would be a discretionary time lock.
The owner could set or unset the times when a file might be accessed.
Stored as an xattr, but neither a label nor Mandatory Access Control.
I propose this as an example of why the name maclabel is inappropriate,
because in this case the data involved is neither. Please also consider
that, as horrible as it may seem, an LSM could legitimately require
more than one xattr. A proper Compartmented Mode Workstation, for
example, might have a MAC label and an Information label, and as anyone
familiar with the CMW spec will tell you, they have to be separate.
Granted, the information label is only supposed to be used to indicate
the actual sensitivity of information, but if it's available someone is
going to use it programaticly.
> When this whole thing
> started it was mandated that security attributes be stored in xattrs.
I'll grant you the xattr bit.
> I
> originally had a more convoluted name but after asking around we thought
> this one was better. Not to mention this is a slightly reworked hook
> that was just removed from the LSM since there were no users. While I'm
> open to potentially changing the name the paradigm that we use the xattr
> functionality of linux to handle security labels has been around since
> the beginning of LSM. If we want to revisit that idea I'm willing to do
> it but it needs more people than just you and I to agree to reopen it.
The paradigm is* a security "blob" which is meaningfull only to the
security module proper. This is what allows SELinux to use secids and
Smack to toss around text strings. It's not MAC data and it's not
an NFS label, it's private to the LSM. It makes a lot of sense to use
an xattr to store a blob but, as the AppArmor people have been known
espouse, it's not the only way. The blob could be referenced from a
table using the inode number (it has been done on other systems and
works fine) rather than an xattr, in which case the whole "name" may
be meaningless.
----
* It was when the whole thing started out at least.
Casey Schaufler
[email protected]
On Thu, 2008-02-28 at 12:04 +1100, James Morris wrote:
> On Wed, 27 Feb 2008, David P. Quigley wrote:
>
> > +#ifdef CONFIG_SECURITY
> > +static inline void nfs_fattr_alloc(struct nfs_fattr *fattr, gfp_t flags)
> > +{
> > + fattr->label = kzalloc(NFS4_MAXLABELLEN, flags);
> > + if (fattr->label == NULL)
> > + panic("Can't allocate security label.");
> > + fattr->label_len = NFS4_MAXLABELLEN;
> > +}
>
> A panic here seems like overkill, and also possibly a DoS vector. I
> suggest having the calling code handle the allocation failure gracefully.
>
Good point, I'll put this on the list. I think I remember you mentioned
something about this before but it must have slipped through the cracks.
> > +
> > +#define nfs_fattr_fini(fattr) _nfs_fattr_fini(fattr, __FILE__, __LINE__, __func__)
> > +static inline void _nfs_fattr_fini(struct nfs_fattr *fattr,
> > + const char *file, int line, const char *func)
> > +{
> > + if ((fattr)->label == NULL) {
> > + if (fattr->label_len != 0) {
> > + printk(KERN_WARNING
> > + "%s:%d %s() nfs_fattr label available (%d)\n",
> > + file, line, func,
> > + fattr->label_len);
> > + }
> > + } else {
> > + if (fattr->label_len == NFS4_MAXLABELLEN)
> > + printk(KERN_WARNING
> > + "%s:%d %s() nfs_fattr label unused\n",
> > + file, line, func);
> > + else if (fattr->label_len != (strlen(fattr->label) + 1))
> > + printk(KERN_WARNING
> > + "%s:%d %s() nfs_fattr label size mismatch (label_len %d, strlen %d)\n",
> > + file, line, func,
> > + fattr->label_len, strlen(fattr->label) + 1);
> > +
> > + kfree(fattr->label);
> > + fattr->label = NULL;
> > + fattr->label_len = 0;
> > + }
> > +}
> > +#else
> > +#define nfs_fattr_alloc(fattr, flags)
> > +#define nfs_fattr_fini(fattr)
> > +#endif
>
> Perhaps introduce a debug configuration option for this code.
In what way? We can change them to respond to the NFS/NFSD debug
variables that are set through proc, or did you have something else in
mind?
>
>
> - James
On Wed, 27 Feb 2008, Dave Quigley wrote:
> > Perhaps introduce a debug configuration option for this code.
>
> In what way? We can change them to respond to the NFS/NFSD debug
> variables that are set through proc, or did you have something else in
> mind?
I was thinking something along the lines of CONFIG_NETFILTER_DEBUG, but I
guess you can also use the NFS debug facilities.
--
James Morris
<[email protected]>
On Wed, 2008-02-27 at 17:07 -0800, Casey Schaufler wrote:
> --- Dave Quigley <[email protected]> wrote:
>
> >
> > On Wed, 2008-02-27 at 15:42 -0800, Casey Schaufler wrote:
> > > --- "David P. Quigley" <[email protected]> wrote:
> > >
> > > ...
> > > > + const char *(*maclabel_getname) (void);
> > >
> > > I think that calling this a maclabel is a really bad idea. For one
> > > thing, it assumes that all interesting security attributes are for
> > > Mandatory Access Control. Also, it assumes that they are stored as
> > > xattrs. While these conditions are both met by the two current LSMs
> > > I would suggest that this is not a fair assumption for the long
> > > haul unless the intention is to lock the lSM into only supporting
> > > xattr based label based MAC modules.
> >
> > Actually that is a completely fair assumption.
>
> A completely reasonable LSM would be a discretionary time lock.
> The owner could set or unset the times when a file might be accessed.
> Stored as an xattr, but neither a label nor Mandatory Access Control.
> I propose this as an example of why the name maclabel is inappropriate,
> because in this case the data involved is neither. Please also consider
> that, as horrible as it may seem, an LSM could legitimately require
> more than one xattr. A proper Compartmented Mode Workstation, for
> example, might have a MAC label and an Information label, and as anyone
> familiar with the CMW spec will tell you, they have to be separate.
> Granted, the information label is only supposed to be used to indicate
> the actual sensitivity of information, but if it's available someone is
> going to use it programaticly.
>
> > When this whole thing
> > started it was mandated that security attributes be stored in xattrs.
>
> I'll grant you the xattr bit.
>
> > I
> > originally had a more convoluted name but after asking around we thought
> > this one was better. Not to mention this is a slightly reworked hook
> > that was just removed from the LSM since there were no users. While I'm
> > open to potentially changing the name the paradigm that we use the xattr
> > functionality of linux to handle security labels has been around since
> > the beginning of LSM. If we want to revisit that idea I'm willing to do
> > it but it needs more people than just you and I to agree to reopen it.
>
> The paradigm is* a security "blob" which is meaningfull only to the
> security module proper. This is what allows SELinux to use secids and
> Smack to toss around text strings. It's not MAC data and it's not
> an NFS label, it's private to the LSM. It makes a lot of sense to use
> an xattr to store a blob but, as the AppArmor people have been known
> espouse, it's not the only way. The blob could be referenced from a
> table using the inode number (it has been done on other systems and
> works fine) rather than an xattr, in which case the whole "name" may
> be meaningless.
I think it might help for you to look at how the hook is actually used.
It is specific to MAC labeling, and we do not want some random other
security attribute name returned here that is for some purpose other
than MAC labeling, like security.capability.
--
Stephen Smalley
National Security Agency
--- Stephen Smalley <[email protected]> wrote:
>
> ...
> > The paradigm is* a security "blob" which is meaningfull only to the
> > security module proper. This is what allows SELinux to use secids and
> > Smack to toss around text strings. It's not MAC data and it's not
> > an NFS label, it's private to the LSM. It makes a lot of sense to use
> > an xattr to store a blob but, as the AppArmor people have been known
> > espouse, it's not the only way. The blob could be referenced from a
> > table using the inode number (it has been done on other systems and
> > works fine) rather than an xattr, in which case the whole "name" may
> > be meaningless.
>
> I think it might help for you to look at how the hook is actually used.
> It is specific to MAC labeling, and we do not want some random other
> security attribute name returned here that is for some purpose other
> than MAC labeling, like security.capability.
I can see how it's being used just fine, thank you.
If you only want this interface for SELinux put it in
SELinux. Don't clutter up the LSM with it. If it's an
LSM interface it should be potentially useful for any
and all LSMs, be they label based or not, MAC or DAC.
Even within a label based MAC scheme it may not be
sensible, given that a MAC scheme could use multiple
xattrs (e.g. a B&L sensitivity label and a Biba integrity
label) to store its blob.
If what you want in LSM terms is a name to give the blob
make your interface be security_blob_name(). The LSM can
deal with this as it sees fit, and NFS can determine if
it's a blob that it wants to deal with independently.
Such an interface could even support stacking should
that ever come about.
LSM is not supposed to be only for MAC and it's not supposed
to be only for label based schemes. It's supposed to be
for additional security restrictions. Providing an interface
that should be generally applicable with a name that
constrains it to a specific subset of those schemes is wrong.
Casey Schaufler
[email protected]
On Thu, 2008-02-28 at 11:23 -0800, Casey Schaufler wrote:
> --- Stephen Smalley <[email protected]> wrote:
>
> >
> > ...
> > > The paradigm is* a security "blob" which is meaningfull only to the
> > > security module proper. This is what allows SELinux to use secids and
> > > Smack to toss around text strings. It's not MAC data and it's not
> > > an NFS label, it's private to the LSM. It makes a lot of sense to use
> > > an xattr to store a blob but, as the AppArmor people have been known
> > > espouse, it's not the only way. The blob could be referenced from a
> > > table using the inode number (it has been done on other systems and
> > > works fine) rather than an xattr, in which case the whole "name" may
> > > be meaningless.
> >
> > I think it might help for you to look at how the hook is actually used.
> > It is specific to MAC labeling, and we do not want some random other
> > security attribute name returned here that is for some purpose other
> > than MAC labeling, like security.capability.
>
> I can see how it's being used just fine, thank you.
> If you only want this interface for SELinux put it in
> SELinux. Don't clutter up the LSM with it. If it's an
> LSM interface it should be potentially useful for any
> and all LSMs, be they label based or not, MAC or DAC.
> Even within a label based MAC scheme it may not be
> sensible, given that a MAC scheme could use multiple
> xattrs (e.g. a B&L sensitivity label and a Biba integrity
> label) to store its blob.
>
> If what you want in LSM terms is a name to give the blob
> make your interface be security_blob_name(). The LSM can
> deal with this as it sees fit, and NFS can determine if
> it's a blob that it wants to deal with independently.
> Such an interface could even support stacking should
> that ever come about.
>
> LSM is not supposed to be only for MAC and it's not supposed
> to be only for label based schemes. It's supposed to be
> for additional security restrictions. Providing an interface
> that should be generally applicable with a name that
> constrains it to a specific subset of those schemes is wrong.
Casey, you aren't listening (why am I surprised?).
This is an interface to be used by NFS to get information from the
security module. The information desired is specific to the MAC
labeling functionality in NFSv4 that is being proposed. That
functionality is MAC specific (necessarily so, just like the ACL
functionality is ACL specific). We are hiding the SELinux-specific bits
behind the LSM interface, and non-MAC LSMs are free to return NULL in
order to indicate that they don't support MAC labeling. We do NOT want
the capability module to return its security blob here, or any other
non-MAC LSM - it will yield the wrong semantics for the NFS MAC support.
In any event, I don't think we need your permission.
--
Stephen Smalley
National Security Agency
--- Stephen Smalley <[email protected]> wrote:
>
> On Thu, 2008-02-28 at 11:23 -0800, Casey Schaufler wrote:
> ...
> > LSM is not supposed to be only for MAC and it's not supposed
> > to be only for label based schemes. It's supposed to be
> > for additional security restrictions. Providing an interface
> > that should be generally applicable with a name that
> > constrains it to a specific subset of those schemes is wrong.
>
> Casey, you aren't listening (why am I surprised?).
I think that I am listening, and I appologize for doing
such a poor job of getting my view on the across.
> This is an interface to be used by NFS to get information from the
> security module. The information desired is specific to the MAC
> labeling functionality in NFSv4 that is being proposed.
Do you understand that if the functionality being proposed
is specific to a particular file system it ought to be contained
in that file system, not proposed as a part of the general
purpose interface?
> That
> functionality is MAC specific (necessarily so, just like the ACL
> functionality is ACL specific).
The ACL funtionality over NFS could be done using general interfaces,
and there are examples (e.g. Irix) where it has been done. I
understand the rationale for the current implementation while
disagreeing with that rationale. Further, there is a major difference
between ACLs and a legitimate LSM (for MAC or DAC) in that ACLs
are a change to the Linux access control scheme (they interact with
the mode bits) whereas a legitimate LSM is strictly additional
restrictions.
> We are hiding the SELinux-specific bits
> behind the LSM interface, and non-MAC LSMs are free to return NULL in
> order to indicate that they don't support MAC labeling. We do NOT want
> the capability module to return its security blob here, or any other
> non-MAC LSM - it will yield the wrong semantics for the NFS MAC support.
I should hope then that your SELinux specific NFS server should
look at the name presented and treat it appropriately.
> In any event, I don't think we need your permission.
You're correct, you don't. You can propose anything you like.
Don't take my criticisms personally, but I think you're wrong
on this one. I don't like to see this unnecessary limitation,
the kind that could haunt the code base for years, when it seems
pretty obvious that it could be better.
Casey Schaufler
[email protected]
Ignoring the security parts of it, here are a few comments about the VFS and
coding style related bits...
Josef 'Jeff' Sipek.
On Wed, Feb 27, 2008 at 03:39:38PM -0500, David P. Quigley wrote:
> This patch adds two new fields to the iattr structure. The first field holds a
> security label while the second contains the length of this label. In addition
> the patch adds a new helper function inode_setsecurity which calls the LSM to
> set the security label on the inode. Finally the patch modifies the necessary
> functions such that fsnotify_change can handle notification requests for
> dnotify and inotify.
>
> Signed-off-by: David P. Quigley <[email protected]>
> ---
> fs/attr.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> fs/xattr.c | 33 +++++++++++++++++++++++++++------
> include/linux/fcntl.h | 1 +
> include/linux/fs.h | 11 +++++++++++
> include/linux/fsnotify.h | 6 ++++++
> include/linux/inotify.h | 3 ++-
> include/linux/xattr.h | 1 +
> 7 files changed, 91 insertions(+), 7 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 966b73e..1df6603 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -5,6 +5,7 @@
> * changes by Thomas Schoebel-Theuer
> */
>
> +#include <linux/fs.h>
> #include <linux/module.h>
> #include <linux/time.h>
> #include <linux/mm.h>
> @@ -14,9 +15,35 @@
> #include <linux/fcntl.h>
> #include <linux/quotaops.h>
> #include <linux/security.h>
> +#include <linux/xattr.h>
>
> /* Taken over from the old code... */
>
> +#ifdef CONFIG_SECURITY
> +/*
> + * Update the in core label.
> + */
> +int inode_setsecurity(struct inode *inode, struct iattr *attr)
> +{
> + const char *suffix = security_maclabel_getname()
> + + XATTR_SECURITY_PREFIX_LEN;
> + int error;
> +
> + if (!attr->ia_valid & ATTR_SECURITY_LABEL)
> + return -EINVAL;
You most likely want:
if (!(attr->ia_valid & ATTR_SECURITY_LABEL))
IOW, watch out for operator precedence.
> +
> + error = security_inode_setsecurity(inode, suffix, attr->ia_label,
> + attr->ia_label_len, 0);
> + if (error)
> + printk(KERN_ERR "%s() %s %d security_inode_setsecurity() %d\n"
> + , __func__, (char *)attr->ia_label, attr->ia_label_len
> + , error);
The commas should really be on the previous line.
> +
> + return error;
> +}
> +EXPORT_SYMBOL(inode_setsecurity);
> +#endif
> +
> /* POSIX UID/GID verification for setting inode attributes. */
> int inode_change_ok(struct inode *inode, struct iattr *attr)
> {
> @@ -94,6 +121,10 @@ int inode_setattr(struct inode * inode, struct iattr * attr)
> mode &= ~S_ISGID;
> inode->i_mode = mode;
> }
> +#ifdef CONFIG_SECURITY
> + if (ia_valid & ATTR_SECURITY_LABEL)
> + inode_setsecurity(inode, attr);
> +#endif
> mark_inode_dirty(inode);
>
> return 0;
> @@ -157,6 +188,18 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
> if (ia_valid & ATTR_SIZE)
> down_write(&dentry->d_inode->i_alloc_sem);
>
> +#ifdef CONFIG_SECURITY
> + if (ia_valid & ATTR_SECURITY_LABEL) {
> + char *key = (char *)security_maclabel_getname();
> + vfs_setxattr_locked(dentry, key,
> + attr->ia_label, attr->ia_label_len, 0);
> + /* Avoid calling inode_setsecurity()
> + * via inode_setattr() below
> + */
> + attr->ia_valid &= ~ATTR_SECURITY_LABEL;
> + }
> +#endif
> +
> if (inode->i_op && inode->i_op->setattr) {
> error = security_inode_setattr(dentry, attr);
> if (!error)
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 3acab16..b5a91e1 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -67,9 +67,9 @@ xattr_permission(struct inode *inode, const char *name, int mask)
> return permission(inode, mask, NULL);
> }
>
> -int
> -vfs_setxattr(struct dentry *dentry, char *name, void *value,
> - size_t size, int flags)
> +static int
> +_vfs_setxattr(struct dentry *dentry, char *name, void *value,
> + size_t size, int flags, int lock)
The convention is to use __ or do_ as the prefix.
> {
> struct inode *inode = dentry->d_inode;
> int error;
> @@ -78,7 +78,8 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> if (error)
> return error;
>
> - mutex_lock(&inode->i_mutex);
> + if (lock)
> + mutex_lock(&inode->i_mutex);
> error = security_inode_setxattr(dentry, name, value, size, flags);
> if (error)
> goto out;
> @@ -95,15 +96,35 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> error = security_inode_setsecurity(inode, suffix, value,
> size, flags);
> - if (!error)
> + if (!error) {
> +#ifdef CONFIG_SECURITY
> + fsnotify_change(dentry, ATTR_SECURITY_LABEL);
> +#endif
> fsnotify_xattr(dentry);
> + }
> }
> out:
> - mutex_unlock(&inode->i_mutex);
> + if (lock)
> + mutex_unlock(&inode->i_mutex);
> return error;
> }
> +
> +int
> +vfs_setxattr(struct dentry *dentry, char *name, void *value,
> + size_t size, int flags)
> +{
> + return _vfs_setxattr(dentry, name, value, size, flags, 1);
> +}
> EXPORT_SYMBOL_GPL(vfs_setxattr);
>
> +int
> +vfs_setxattr_locked(struct dentry *dentry, char *name, void *value,
> + size_t size, int flags)
> +{
> + return _vfs_setxattr(dentry, name, value, size, flags, 0);
> +}
> +EXPORT_SYMBOL_GPL(vfs_setxattr_locked);
Alright...so, few things...
1) why do you need the locked/unlocked versions?
2) instead of passing a flag to a common function, why not have:
vfs_setxattr_locked(....)
{
// original code minus the lock/unlock calls
}
vfs_setxattr(....)
{
mutex_lock(...);
vfs_setxattr_locked(...);
mutex_unlock(...);
}
> +
> ssize_t
> xattr_getsecurity(struct inode *inode, const char *name, void *value,
> size_t size)
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index 8603740..fae1e59 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -30,6 +30,7 @@
> #define DN_DELETE 0x00000008 /* File removed */
> #define DN_RENAME 0x00000010 /* File renamed */
> #define DN_ATTRIB 0x00000020 /* File changed attibutes */
> +#define DN_LABEL 0x00000040 /* File (re)labeled */
> #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
>
> #define AT_FDCWD -100 /* Special value used to indicate
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b84b848..757add6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -335,6 +335,10 @@ typedef void (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> #define ATTR_KILL_PRIV 16384
> #define ATTR_OPEN 32768 /* Truncating from open(O_TRUNC) */
>
> +#ifdef CONFIG_SECURITY
> +#define ATTR_SECURITY_LABEL 65536
> +#endif
> +
> /*
> * This is the Inode Attributes structure, used for notify_change(). It
> * uses the above definitions as flags, to know which values have changed.
> @@ -360,6 +364,10 @@ struct iattr {
> * check for (ia_valid & ATTR_FILE), and not for (ia_file != NULL).
> */
> struct file *ia_file;
> +#ifdef CONFIG_SECURITY
> + void *ia_label;
> + u32 ia_label_len;
> +#endif
> };
>
> /*
> @@ -1971,6 +1979,9 @@ extern int buffer_migrate_page(struct address_space *,
> #define buffer_migrate_page NULL
> #endif
>
> +#ifdef CONFIG_SECURITY
> +extern int inode_setsecurity(struct inode *inode, struct iattr *attr);
> +#endif
> extern int inode_change_ok(struct inode *, struct iattr *);
> extern int __must_check inode_setattr(struct inode *, struct iattr *);
>
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index d4b7c4a..b54a3cb 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -253,6 +253,12 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
> in_mask |= IN_ATTRIB;
> dn_mask |= DN_ATTRIB;
> }
> +#ifdef CONFIG_SECURITY
> + if (ia_valid & ATTR_SECURITY_LABEL) {
> + in_mask |= IN_LABEL;
> + dn_mask |= DN_LABEL;
> + }
> +#endif
>
> if (dn_mask)
> dnotify_parent(dentry, dn_mask);
> diff --git a/include/linux/inotify.h b/include/linux/inotify.h
> index 742b917..10f3ace 100644
> --- a/include/linux/inotify.h
> +++ b/include/linux/inotify.h
> @@ -36,6 +36,7 @@ struct inotify_event {
> #define IN_DELETE 0x00000200 /* Subfile was deleted */
> #define IN_DELETE_SELF 0x00000400 /* Self was deleted */
> #define IN_MOVE_SELF 0x00000800 /* Self was moved */
> +#define IN_LABEL 0x00001000 /* Self was (re)labeled */
>
> /* the following are legal events. they are sent as needed to any watch */
> #define IN_UNMOUNT 0x00002000 /* Backing fs was unmounted */
> @@ -61,7 +62,7 @@ struct inotify_event {
> #define IN_ALL_EVENTS (IN_ACCESS | IN_MODIFY | IN_ATTRIB | IN_CLOSE_WRITE | \
> IN_CLOSE_NOWRITE | IN_OPEN | IN_MOVED_FROM | \
> IN_MOVED_TO | IN_DELETE | IN_CREATE | IN_DELETE_SELF | \
> - IN_MOVE_SELF)
> + IN_MOVE_SELF | IN_LABEL)
>
> #ifdef __KERNEL__
>
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index df6b95d..1169963 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -50,6 +50,7 @@ ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
> ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
> ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
> int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
> +int vfs_setxattr_locked(struct dentry *, char *, void *, size_t, int);
> int vfs_removexattr(struct dentry *, char *);
>
> ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
> --
> 1.5.3.8
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Don't drink and derive. Alcohol and algebra don't mix.
On Thu, 2008-02-28 at 12:04 +1100, James Morris wrote:
> On Wed, 27 Feb 2008, David P. Quigley wrote:
>
> > +#ifdef CONFIG_SECURITY
> > +static inline void nfs_fattr_alloc(struct nfs_fattr *fattr, gfp_t flags)
> > +{
> > + fattr->label = kzalloc(NFS4_MAXLABELLEN, flags);
> > + if (fattr->label == NULL)
> > + panic("Can't allocate security label.");
> > + fattr->label_len = NFS4_MAXLABELLEN;
> > +}
>
> A panic here seems like overkill, and also possibly a DoS vector. I
> suggest having the calling code handle the allocation failure gracefully.
>
> > +
> > +#define nfs_fattr_fini(fattr) _nfs_fattr_fini(fattr, __FILE__, __LINE__, __func__)
> > +static inline void _nfs_fattr_fini(struct nfs_fattr *fattr,
> > + const char *file, int line, const char *func)
> > +{
> > + if ((fattr)->label == NULL) {
> > + if (fattr->label_len != 0) {
> > + printk(KERN_WARNING
> > + "%s:%d %s() nfs_fattr label available (%d)\n",
> > + file, line, func,
> > + fattr->label_len);
> > + }
> > + } else {
> > + if (fattr->label_len == NFS4_MAXLABELLEN)
> > + printk(KERN_WARNING
> > + "%s:%d %s() nfs_fattr label unused\n",
> > + file, line, func);
> > + else if (fattr->label_len != (strlen(fattr->label) + 1))
> > + printk(KERN_WARNING
> > + "%s:%d %s() nfs_fattr label size mismatch (label_len %d, strlen %d)\n",
> > + file, line, func,
> > + fattr->label_len, strlen(fattr->label) + 1);
> > +
> > + kfree(fattr->label);
> > + fattr->label = NULL;
> > + fattr->label_len = 0;
> > + }
> > +}
> > +#else
> > +#define nfs_fattr_alloc(fattr, flags)
> > +#define nfs_fattr_fini(fattr)
> > +#endif
>
> Perhaps introduce a debug configuration option for this code.
>
>
> - James
A question about the debug configuration here. Exactly what information
are we looking to get for debugging. If we want line/file/function that
these are called on then I need a macro wrapper for the allocation as
well. If that is the case I'm guessing we always define the macros
nfs_fattr_alloc and nfs_fattr_fini and just make the internal functions
static inline so they can be compiled away when !CONFIG_SECURITY.
Dave
On Thu, 2008-02-28 at 15:10 -0500, Josef 'Jeff' Sipek wrote:
> Ignoring the security parts of it, here are a few comments about the VFS and
> coding style related bits...
>
> Josef 'Jeff' Sipek.
>
> On Wed, Feb 27, 2008 at 03:39:38PM -0500, David P. Quigley wrote:
> > This patch adds two new fields to the iattr structure. The first field holds a
> > security label while the second contains the length of this label. In addition
> > the patch adds a new helper function inode_setsecurity which calls the LSM to
> > set the security label on the inode. Finally the patch modifies the necessary
> > functions such that fsnotify_change can handle notification requests for
> > dnotify and inotify.
> >
> > Signed-off-by: David P. Quigley <[email protected]>
> > ---
> > fs/attr.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> > fs/xattr.c | 33 +++++++++++++++++++++++++++------
> > include/linux/fcntl.h | 1 +
> > include/linux/fs.h | 11 +++++++++++
> > include/linux/fsnotify.h | 6 ++++++
> > include/linux/inotify.h | 3 ++-
> > include/linux/xattr.h | 1 +
> > 7 files changed, 91 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index 966b73e..1df6603 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -5,6 +5,7 @@
> > * changes by Thomas Schoebel-Theuer
> > */
> >
> > +#include <linux/fs.h>
> > #include <linux/module.h>
> > #include <linux/time.h>
> > #include <linux/mm.h>
> > @@ -14,9 +15,35 @@
> > #include <linux/fcntl.h>
> > #include <linux/quotaops.h>
> > #include <linux/security.h>
> > +#include <linux/xattr.h>
> >
> > /* Taken over from the old code... */
> >
> > +#ifdef CONFIG_SECURITY
> > +/*
> > + * Update the in core label.
> > + */
> > +int inode_setsecurity(struct inode *inode, struct iattr *attr)
> > +{
> > + const char *suffix = security_maclabel_getname()
> > + + XATTR_SECURITY_PREFIX_LEN;
> > + int error;
> > +
> > + if (!attr->ia_valid & ATTR_SECURITY_LABEL)
> > + return -EINVAL;
>
> You most likely want:
>
> if (!(attr->ia_valid & ATTR_SECURITY_LABEL))
>
> IOW, watch out for operator precedence.
Already raised by James and fixed but thanks for catching it again.
>
> > +
> > + error = security_inode_setsecurity(inode, suffix, attr->ia_label,
> > + attr->ia_label_len, 0);
> > + if (error)
> > + printk(KERN_ERR "%s() %s %d security_inode_setsecurity() %d\n"
> > + , __func__, (char *)attr->ia_label, attr->ia_label_len
> > + , error);
>
> The commas should really be on the previous line.
Fixed.
>
> > +
> > + return error;
> > +}
> > +EXPORT_SYMBOL(inode_setsecurity);
> > +#endif
> > +
> > /* POSIX UID/GID verification for setting inode attributes. */
> > int inode_change_ok(struct inode *inode, struct iattr *attr)
> > {
> > @@ -94,6 +121,10 @@ int inode_setattr(struct inode * inode, struct iattr * attr)
> > mode &= ~S_ISGID;
> > inode->i_mode = mode;
> > }
> > +#ifdef CONFIG_SECURITY
> > + if (ia_valid & ATTR_SECURITY_LABEL)
> > + inode_setsecurity(inode, attr);
> > +#endif
> > mark_inode_dirty(inode);
> >
> > return 0;
> > @@ -157,6 +188,18 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
> > if (ia_valid & ATTR_SIZE)
> > down_write(&dentry->d_inode->i_alloc_sem);
> >
> > +#ifdef CONFIG_SECURITY
> > + if (ia_valid & ATTR_SECURITY_LABEL) {
> > + char *key = (char *)security_maclabel_getname();
> > + vfs_setxattr_locked(dentry, key,
> > + attr->ia_label, attr->ia_label_len, 0);
> > + /* Avoid calling inode_setsecurity()
> > + * via inode_setattr() below
> > + */
> > + attr->ia_valid &= ~ATTR_SECURITY_LABEL;
> > + }
> > +#endif
> > +
> > if (inode->i_op && inode->i_op->setattr) {
> > error = security_inode_setattr(dentry, attr);
> > if (!error)
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 3acab16..b5a91e1 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -67,9 +67,9 @@ xattr_permission(struct inode *inode, const char *name, int mask)
> > return permission(inode, mask, NULL);
> > }
> >
> > -int
> > -vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > - size_t size, int flags)
> > +static int
> > +_vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > + size_t size, int flags, int lock)
>
> The convention is to use __ or do_ as the prefix.
Fixed (added another _ to the beginning.)
>
> > {
> > struct inode *inode = dentry->d_inode;
> > int error;
> > @@ -78,7 +78,8 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > if (error)
> > return error;
> >
> > - mutex_lock(&inode->i_mutex);
> > + if (lock)
> > + mutex_lock(&inode->i_mutex);
> > error = security_inode_setxattr(dentry, name, value, size, flags);
> > if (error)
> > goto out;
> > @@ -95,15 +96,35 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> > error = security_inode_setsecurity(inode, suffix, value,
> > size, flags);
> > - if (!error)
> > + if (!error) {
> > +#ifdef CONFIG_SECURITY
> > + fsnotify_change(dentry, ATTR_SECURITY_LABEL);
> > +#endif
> > fsnotify_xattr(dentry);
> > + }
> > }
> > out:
> > - mutex_unlock(&inode->i_mutex);
> > + if (lock)
> > + mutex_unlock(&inode->i_mutex);
> > return error;
> > }
> > +
> > +int
> > +vfs_setxattr(struct dentry *dentry, char *name, void *value,
> > + size_t size, int flags)
> > +{
> > + return _vfs_setxattr(dentry, name, value, size, flags, 1);
> > +}
> > EXPORT_SYMBOL_GPL(vfs_setxattr);
> >
> > +int
> > +vfs_setxattr_locked(struct dentry *dentry, char *name, void *value,
> > + size_t size, int flags)
> > +{
> > + return _vfs_setxattr(dentry, name, value, size, flags, 0);
> > +}
> > +EXPORT_SYMBOL_GPL(vfs_setxattr_locked);
>
> Alright...so, few things...
>
> 1) why do you need the locked/unlocked versions?
>
> 2) instead of passing a flag to a common function, why not have:
>
> vfs_setxattr_locked(....)
> {
> // original code minus the lock/unlock calls
> }
>
> vfs_setxattr(....)
> {
> mutex_lock(...);
> vfs_setxattr_locked(...);
> mutex_unlock(...);
> }
What we do and what you propose aren't logically equivalent. There is a
permission check inside vfs_setxattr before the mutex lock. However
looking at through the xattr_permission function and its call chain it
doesn't seem like we would create a deadlock by locking the inode before
it is called; so it is possible to do what you propose. Since setting of
xattrs (at least from our perspective) is a less common operation I
don't think putting locking around the entire call would make that large
of a difference.
Dave
On Thu, Feb 28, 2008 at 03:39:30PM -0500, Dave Quigley wrote:
...
> > Alright...so, few things...
> >
> > 1) why do you need the locked/unlocked versions?
> >
> > 2) instead of passing a flag to a common function, why not have:
> >
> > vfs_setxattr_locked(....)
> > {
> > // original code minus the lock/unlock calls
> > }
> >
> > vfs_setxattr(....)
> > {
> > mutex_lock(...);
> > vfs_setxattr_locked(...);
> > mutex_unlock(...);
> > }
>
> What we do and what you propose aren't logically equivalent. There is a
> permission check inside vfs_setxattr before the mutex lock.
Ah, right. I didn't notice the @@ line...
Josef 'Jeff' Sipek.
--
Keyboard not found!
Press F1 to enter Setup
On Thu, 2008-02-28 at 16:15 -0500, Josef 'Jeff' Sipek wrote:
> On Thu, Feb 28, 2008 at 03:39:30PM -0500, Dave Quigley wrote:
> ...
> > > Alright...so, few things...
> > >
> > > 1) why do you need the locked/unlocked versions?
> > >
> > > 2) instead of passing a flag to a common function, why not have:
> > >
> > > vfs_setxattr_locked(....)
> > > {
> > > // original code minus the lock/unlock calls
> > > }
> > >
> > > vfs_setxattr(....)
> > > {
> > > mutex_lock(...);
> > > vfs_setxattr_locked(...);
> > > mutex_unlock(...);
> > > }
> >
> > What we do and what you propose aren't logically equivalent. There is a
> > permission check inside vfs_setxattr before the mutex lock.
>
> Ah, right. I didn't notice the @@ line...
>
> Josef 'Jeff' Sipek.
>
I'm compiling a test kernel with your proposed change to make sure it
doesn't deadlock. If it works then I'll go with your solution since its
less messy.
Dave
On Thu, Feb 28, 2008 at 04:05:11PM -0500, Dave Quigley wrote:
>
> On Thu, 2008-02-28 at 16:15 -0500, Josef 'Jeff' Sipek wrote:
> > On Thu, Feb 28, 2008 at 03:39:30PM -0500, Dave Quigley wrote:
> > ...
> > > > Alright...so, few things...
> > > >
> > > > 1) why do you need the locked/unlocked versions?
> > > >
> > > > 2) instead of passing a flag to a common function, why not have:
> > > >
> > > > vfs_setxattr_locked(....)
> > > > {
> > > > // original code minus the lock/unlock calls
> > > > }
> > > >
> > > > vfs_setxattr(....)
> > > > {
> > > > mutex_lock(...);
> > > > vfs_setxattr_locked(...);
> > > > mutex_unlock(...);
> > > > }
> > >
> > > What we do and what you propose aren't logically equivalent. There is a
> > > permission check inside vfs_setxattr before the mutex lock.
> >
> > Ah, right. I didn't notice the @@ line...
> >
> > Josef 'Jeff' Sipek.
> >
>
> I'm compiling a test kernel with your proposed change to make sure it
> doesn't deadlock. If it works then I'll go with your solution since its
> less messy.
I glanced at the call chain, and this one is making me uneasy:
vfs_setxattr => xattr_permission => permission => inode_op->permission
But Documentation/filesystems/Locking says that the inode permission op
doesn't need an i_mutex, so things should be ok.
Josef 'Jeff' Sipek.
--
Failure is not an option,
It comes bundled with your Microsoft product.
On Thu, 2008-02-28 at 16:39 -0500, Josef 'Jeff' Sipek wrote:
> On Thu, Feb 28, 2008 at 04:05:11PM -0500, Dave Quigley wrote:
> >
> > On Thu, 2008-02-28 at 16:15 -0500, Josef 'Jeff' Sipek wrote:
> > > On Thu, Feb 28, 2008 at 03:39:30PM -0500, Dave Quigley wrote:
> > > ...
> > > > > Alright...so, few things...
> > > > >
> > > > > 1) why do you need the locked/unlocked versions?
> > > > >
> > > > > 2) instead of passing a flag to a common function, why not have:
> > > > >
> > > > > vfs_setxattr_locked(....)
> > > > > {
> > > > > // original code minus the lock/unlock calls
> > > > > }
> > > > >
> > > > > vfs_setxattr(....)
> > > > > {
> > > > > mutex_lock(...);
> > > > > vfs_setxattr_locked(...);
> > > > > mutex_unlock(...);
> > > > > }
> > > >
> > > > What we do and what you propose aren't logically equivalent. There is a
> > > > permission check inside vfs_setxattr before the mutex lock.
> > >
> > > Ah, right. I didn't notice the @@ line...
> > >
> > > Josef 'Jeff' Sipek.
> > >
> >
> > I'm compiling a test kernel with your proposed change to make sure it
> > doesn't deadlock. If it works then I'll go with your solution since its
> > less messy.
>
> I glanced at the call chain, and this one is making me uneasy:
>
> vfs_setxattr => xattr_permission => permission => inode_op->permission
>
> But Documentation/filesystems/Locking says that the inode permission op
> doesn't need an i_mutex, so things should be ok.
>
> Josef 'Jeff' Sipek.
>
Yea I saw the same thing. In practice it seems to be correct as well. I
looked at the ext2/3/4 and xfs implementations and they just call
generic_permission which doesn't grab i_mutex.
Dave
On Thu, 28 Feb 2008, Dave Quigley wrote:
> A question about the debug configuration here. Exactly what information
> are we looking to get for debugging. If we want line/file/function that
> these are called on then I need a macro wrapper for the allocation as
> well. If that is the case I'm guessing we always define the macros
> nfs_fattr_alloc and nfs_fattr_fini and just make the internal functions
> static inline so they can be compiled away when !CONFIG_SECURITY.
Well, a WARN_ON will give you a stack trace. It's up to you -- how much
debugging do you think this will need now?
- James
--
James Morris
<[email protected]>
The only benefit I can see for this is to make sure we don't leak any of
these labels. I can't find a reasonable way to match these up though so
it will just give us a hint that we are leaking something.
Dave
On Fri, 2008-02-29 at 10:00 +1100, James Morris wrote:
> On Thu, 28 Feb 2008, Dave Quigley wrote:
>
> > A question about the debug configuration here. Exactly what information
> > are we looking to get for debugging. If we want line/file/function that
> > these are called on then I need a macro wrapper for the allocation as
> > well. If that is the case I'm guessing we always define the macros
> > nfs_fattr_alloc and nfs_fattr_fini and just make the internal functions
> > static inline so they can be compiled away when !CONFIG_SECURITY.
>
> Well, a WARN_ON will give you a stack trace. It's up to you -- how much
> debugging do you think this will need now?
>
>
> - James
On Thu, Feb 28, 2008 at 02:30:35PM -0500, Stephen Smalley wrote:
> This is an interface to be used by NFS to get information from the
> security module. The information desired is specific to the MAC
> labeling functionality in NFSv4 that is being proposed. That
> functionality is MAC specific (necessarily so, just like the ACL
> functionality is ACL specific). We are hiding the SELinux-specific bits
> behind the LSM interface, and non-MAC LSMs are free to return NULL in
> order to indicate that they don't support MAC labeling. We do NOT want
> the capability module to return its security blob here, or any other
> non-MAC LSM - it will yield the wrong semantics for the NFS MAC support.
I think Casey is totally right here. The LSM interface should not be
as specific here. If you want to limit the NFSv4 interface to single
MAC xattr label based systems add an additional method to check if
the LSM is that. But the proper fix is of course to not add somthing
so specific to NFSv4 at all, as it's got enough shortcoming already.
Please add a proper xattr protocol. It's not like it's hard, SGI
has been doing this in IRIX for NFSv3 for ages as a sideband protocol,
and even release the reference source under the GPL. Just either use
that with NFSv4 or if you feel fancy merge it into the NFS spec for
NFSv6^H4.2.
> In any event, I don't think we need your permission.
Wow, that's rude even to someone as direct as me. Casey is the only
other person having an in-tree LSM, and I think his input in this
area is important. But if not I as a VFS person can happily give
you my "no" for the current version from the VFS point of view.
On Thu, 2008-02-28 at 18:48 -0500, Christoph Hellwig wrote:
> On Thu, Feb 28, 2008 at 02:30:35PM -0500, Stephen Smalley wrote:
> > This is an interface to be used by NFS to get information from the
> > security module. The information desired is specific to the MAC
> > labeling functionality in NFSv4 that is being proposed. That
> > functionality is MAC specific (necessarily so, just like the ACL
> > functionality is ACL specific). We are hiding the SELinux-specific bits
> > behind the LSM interface, and non-MAC LSMs are free to return NULL in
> > order to indicate that they don't support MAC labeling. We do NOT want
> > the capability module to return its security blob here, or any other
> > non-MAC LSM - it will yield the wrong semantics for the NFS MAC support.
>
> I think Casey is totally right here. The LSM interface should not be
> as specific here. If you want to limit the NFSv4 interface to single
> MAC xattr label based systems add an additional method to check if
> the LSM is that. But the proper fix is of course to not add somthing
> so specific to NFSv4 at all, as it's got enough shortcoming already.
> Please add a proper xattr protocol. It's not like it's hard, SGI
> has been doing this in IRIX for NFSv3 for ages as a sideband protocol,
> and even release the reference source under the GPL. Just either use
> that with NFSv4 or if you feel fancy merge it into the NFS spec for
> NFSv6^H4.2.
There are several things here. I've spoken to several people about this
and the belief I've gotten from most of them is that a recommended
attribute is how this is to be transported. The NFSv4 spec people will
probably say that if you want xattr like functionality for NFSv4 use
named attributes. For us this is not an option since we require
semantics to label on create/open and the only way we can do this is by
adding a recommended attribute. The create/open calls in NFSv4 takes a
list of attributes to use on create as part of the request. I really
don't see a difference between the security blob and the
username/groupname that NFSv4 currently uses. Also there is a good
chance that we will need to translate labels at some point (read future
work).
>
> > In any event, I don't think we need your permission.
>
> Wow, that's rude even to someone as direct as me. Casey is the only
> other person having an in-tree LSM, and I think his input in this
> area is important. But if not I as a VFS person can happily give
> you my "no" for the current version from the VFS point of view.
I can only speak for myself but honestly I've only seen Casey act
confrontational to this idea from the beginning. There is absolutely
nothing in here that is SELinux specific, tecnically its not even MAC
specific. I said from the beginning that this was perhaps not the best
name and we are willing to change it. There is nothing in this hook that
wasn't in LSM before. This is almost identical functionality to what
Adrian removed in 2.6.24. The only difference between this and
security_inode_getsuffix is that this returns security.suffix and that
the name is different. I don't have a SMACK box to test it on but I'm
99% sure that if Casey tried to use SMACK with this patch set that he
would have labeled nfs working with SMACK. If it doesn't work with SMACK
right now I'm willing to help him with that and even include it in the
patch set. But spreading FUD about how we are including SELinux specific
code in here is just that.
Dave
On Thu, Feb 28, 2008 at 07:04:57PM -0500, Dave Quigley wrote:
> There are several things here. I've spoken to several people about this
> and the belief I've gotten from most of them is that a recommended
> attribute is how this is to be transported. The NFSv4 spec people will
> probably say that if you want xattr like functionality for NFSv4 use
> named attributes. For us this is not an option since we require
> semantics to label on create/open and the only way we can do this is by
> adding a recommended attribute. The create/open calls in NFSv4 takes a
> list of attributes to use on create as part of the request. I really
> don't see a difference between the security blob and the
> username/groupname that NFSv4 currently uses. Also there is a good
> chance that we will need to translate labels at some point (read future
> work).
Then use the existing side-band protocol and ignore the NFSv4 spec
group. They're <skip colourful language here> anyway.
> > Wow, that's rude even to someone as direct as me. Casey is the only
> > other person having an in-tree LSM, and I think his input in this
> > area is important. But if not I as a VFS person can happily give
> > you my "no" for the current version from the VFS point of view.
>
> I can only speak for myself but honestly I've only seen Casey act
> confrontational to this idea from the beginning. There is absolutely
> nothing in here that is SELinux specific, tecnically its not even MAC
> specific. I said from the beginning that this was perhaps not the best
> name and we are willing to change it
And changing the name and minor details is exactly what Casey requested.
On Thu, 2008-02-28 at 19:39 -0500, Christoph Hellwig wrote:
> On Thu, Feb 28, 2008 at 07:04:57PM -0500, Dave Quigley wrote:
> > There are several things here. I've spoken to several people about this
> > and the belief I've gotten from most of them is that a recommended
> > attribute is how this is to be transported. The NFSv4 spec people will
> > probably say that if you want xattr like functionality for NFSv4 use
> > named attributes. For us this is not an option since we require
> > semantics to label on create/open and the only way we can do this is by
> > adding a recommended attribute. The create/open calls in NFSv4 takes a
> > list of attributes to use on create as part of the request. I really
> > don't see a difference between the security blob and the
> > username/groupname that NFSv4 currently uses. Also there is a good
> > chance that we will need to translate labels at some point (read future
> > work).
>
> Then use the existing side-band protocol and ignore the NFSv4 spec
> group. They're <skip colourful language here> anyway.
As I've told you several times before: we're _NOT_ putting private
ioctl^Hxattrs onto the wire. If the protocol can't be described in an
RFC, then it isn't going in no matter what expletive you choose to
use...
On Thu, Feb 28, 2008 at 04:50:06PM -0800, Trond Myklebust wrote:
> As I've told you several times before: we're _NOT_ putting private
> ioctl^Hxattrs onto the wire. If the protocol can't be described in an
> RFC, then it isn't going in no matter what expletive you choose to
> use...
It's as unstructured as the named attributes already in. Or file data
for that matter.
On Thu, 2008-02-28 at 19:39 -0500, Christoph Hellwig wrote:
> On Thu, Feb 28, 2008 at 07:04:57PM -0500, Dave Quigley wrote:
> > There are several things here. I've spoken to several people about this
> > and the belief I've gotten from most of them is that a recommended
> > attribute is how this is to be transported. The NFSv4 spec people will
> > probably say that if you want xattr like functionality for NFSv4 use
> > named attributes. For us this is not an option since we require
> > semantics to label on create/open and the only way we can do this is by
> > adding a recommended attribute. The create/open calls in NFSv4 takes a
> > list of attributes to use on create as part of the request. I really
> > don't see a difference between the security blob and the
> > username/groupname that NFSv4 currently uses. Also there is a good
> > chance that we will need to translate labels at some point (read future
> > work).
>
> Then use the existing side-band protocol and ignore the NFSv4 spec
> group. They're <skip colourful language here> anyway.
The reason we are trying to go through the standards process in the
first place is that there is a desire to use SELinux with large netapp
storage boxes. I don't believe that netapp supports the existing
side-band protocol for NFSv4 and the impression I got was that they we
were going to have an incredibly hard time convincing them of putting
anything in that isn't part of the standard. It seems that adding one
recommended attribute which is described in a 3 page internet draft(Most
of which is BS that is part of the template) would be significantly
easier to get into the standard than trying to push an out of band xattr
protocol. Also, I believe Trond even expressed some discontent with it a
while back when we first started development.
>
> > > Wow, that's rude even to someone as direct as me. Casey is the only
> > > other person having an in-tree LSM, and I think his input in this
> > > area is important. But if not I as a VFS person can happily give
> > > you my "no" for the current version from the VFS point of view.
> >
> > I can only speak for myself but honestly I've only seen Casey act
> > confrontational to this idea from the beginning. There is absolutely
> > nothing in here that is SELinux specific, tecnically its not even MAC
> > specific. I said from the beginning that this was perhaps not the best
> > name and we are willing to change it
>
> And changing the name and minor details is exactly what Casey requested.
I can always go with the original hook name of get_security_xattr_name
which turns into a security_get_security_xattr_name call which seems a
bit ludicrous. The only other complaint that I saw from Casey besides
the name of the function was that there could be more than one xattr. If
we want to address that then I need another hook that says give me all
data that the LSM deems important for this file. Which is essentially
the same thing as taking each of the xattr names that the module will
provide, grabbing each of them in turn, and concatenating them together.
For SELinux this is no different than getsecurity with the selinux
suffix. The same goes for SMACK.
Dave
On Thu, 2008-02-28 at 19:51 -0500, Christoph Hellwig wrote:
> On Thu, Feb 28, 2008 at 04:50:06PM -0800, Trond Myklebust wrote:
> > As I've told you several times before: we're _NOT_ putting private
> > ioctl^Hxattrs onto the wire. If the protocol can't be described in an
> > RFC, then it isn't going in no matter what expletive you choose to
> > use...
>
> It's as unstructured as the named attributes already in. Or file data
> for that matter.
Describing what is supposed to be a security mechanism in a structured
fashion for use in a protocol should hardly be an impossible task (and
AFAICS, Dave & co are making good progress in doing so). If it is, then
that casts serious doubt on the validity of the security model...
There should be no need for ioctls.
On Thu, Feb 28, 2008 at 07:32:47PM -0500, Dave Quigley wrote:
> I can always go with the original hook name of get_security_xattr_name
> which turns into a security_get_security_xattr_name call which seems a
> bit ludicrous. The only other complaint that I saw from Casey besides
> the name of the function was that there could be more than one xattr. If
> we want to address that then I need another hook that says give me all
> data that the LSM deems important for this file. Which is essentially
> the same thing as taking each of the xattr names that the module will
> provide, grabbing each of them in turn, and concatenating them together.
> For SELinux this is no different than getsecurity with the selinux
> suffix. The same goes for SMACK.
What about Casey's suggestion of get_security_blob? For any reasonable
module that just has a single xattr it's trivial and for those that
have multiple or a different storage model it might get complicated
but that's not our problem for now.
--- Dave Quigley <[email protected]> wrote:
>
> ...
>
> I can only speak for myself but honestly I've only seen Casey act
> confrontational to this idea from the beginning.
That is simply because I don't care for your design and implementation
choices, I think they're a bad way to go, I've suggested what I
think you should do, and I'm sorry that that comes off as
confrontational but that does not change what I see as flaws in
your approach. I understand what you're trying to do and I think
it's wrong.
> There is absolutely
> nothing in here that is SELinux specific, tecnically its not even MAC
> specific.
Then why are you putting "mac" in the interface name?
> I said from the beginning that this was perhaps not the best
> name and we are willing to change it.
If you read back in the thread, that is what I suggested you do.
> There is nothing in this hook that
> wasn't in LSM before. This is almost identical functionality to what
> Adrian removed in 2.6.24. The only difference between this and
> security_inode_getsuffix is that this returns security.suffix and that
> the name is different. I don't have a SMACK box to test it on but I'm
> 99% sure that if Casey tried to use SMACK with this patch set that he
> would have labeled nfs working with SMACK.
You're very possibly right. I am not argueing from what's right for
Smack, I am argueing from what's right for the LSM. Smack is a label
based MAC LSM, like SELinux. I would expect that it would be easy for
the NFS implementation to accomodate both.
> If it doesn't work with SMACK
> right now I'm willing to help him with that and even include it in the
> patch set. But spreading FUD about how we are including SELinux specific
> code in here is just that.
Sorry, but I'm not argueing that it's SELinux specific at this point.
I'm argueing that it's specific to single label stored in an xattr
based MAC systems (a set of which both SELinux and Smack are members)
and that it is file system specific to NFS. Any of these attributes
makes it questionable as an LSM interface.
As I said before, trying to be helpful, call it security_blob_name(),
and the upcoming Discretionary Time Lock module can return NULL,
indicating that it wants to share no blob name. Or call it
security_xattr_names() and DTL can return NULL and B&L+Biba can
return "security.Bell&LaPadula security.Biba", hoping that everyone
who uses the interface accepts the blank seperation as an indication
that there are multiple xattrs involved.
I am saying that security_maclabel() is a bad choice, and I think
that as an LSM (not MAC, not xattr, not NFS) interface it should
serve the LSM, making the LSM interface better first, and being
the specific interface that a particular file system finds
convenient second.
And before we go any further, I have personally been involved in
doing labeled NFS three times, and I know where the bodies are
buried. Your approach is fine for single label stored in xattr based
MAC systems. It does not generalize worth catfish whiskers, whereas
the two other schemes I've done do so flawlessly. I am critical of
this approach only because I know that y'all can do better.
Great. Now I owe the entire labeled NFS team beer.
Thank you.
Casey Schaufler
[email protected]
On Thu, 2008-02-28 at 20:00 -0500, Christoph Hellwig wrote:
> On Thu, Feb 28, 2008 at 07:32:47PM -0500, Dave Quigley wrote:
> > I can always go with the original hook name of get_security_xattr_name
> > which turns into a security_get_security_xattr_name call which seems a
> > bit ludicrous. The only other complaint that I saw from Casey besides
> > the name of the function was that there could be more than one xattr. If
> > we want to address that then I need another hook that says give me all
> > data that the LSM deems important for this file. Which is essentially
> > the same thing as taking each of the xattr names that the module will
> > provide, grabbing each of them in turn, and concatenating them together.
> > For SELinux this is no different than getsecurity with the selinux
> > suffix. The same goes for SMACK.
>
> What about Casey's suggestion of get_security_blob? For any reasonable
> module that just has a single xattr it's trivial and for those that
> have multiple or a different storage model it might get complicated
> but that's not our problem for now.
If this is the method we are going to use then we need a corresponding
set_security_blob as well. This seems like a paradigm shift for
accessing security information in the kernel. I said to Casey in the
beginning that I'd be willing to revisit it but that neither he or I
alone could make the decision. Unless I misunderstood the original
mandate for security information and that it only applies to how user
space accesses it.
Dave
On Thu, 28 Feb 2008, Dave Quigley wrote:
> There are several things here. I've spoken to several people about this
> and the belief I've gotten from most of them is that a recommended
> attribute is how this is to be transported. The NFSv4 spec people will
> probably say that if you want xattr like functionality for NFSv4 use
> named attributes.
NAs are a non-starter here for a couple of reasons.
1. They are specified as being user managed and opaque to NFS. MAC
labels are typically set by the OS, and may only be set by the user when
permitted by MAC policy. The labels need to be interpreted by the OS to
allow MAC policy to be enforced.
2. The NA namespace is arbitrary and opaque to the OS. There's no scope
in NFSv4 design to allow a namespace to be specified for e.g. MAC labels,
and trying to modify the spec to allow it seems impractical to me. It
would at the very least break backward compatibility with clients and
servers, and lead to some ugly hacks to try and ensure that systems were
reliably speaking to peers which understood the namespace.
It might be possible to implement Linux/BSD style xattrs for NFSv4,
assuming that the IETF folk would approve of the idea, but I don't think
this is really the right solution for conveying MAC labels across the
wire. The xattr API as a local interface is pretty good for this (as it
is FS independent, simple, and established), but that does not
automatically translate to an xattr wire protocol being the right thing.
The problem with this, I believe, is that you end up with quite a lot of
overhead and complexity being added to NFSv4 which does not actually meet
the requirements of MAC labeling, and like NAs, seems more suited
for arbitrary user-managed metdata.
Using RAs for MAC labels seems most appropriate, as they're simple,
extensible and already used for similar protocol attributes such as ACLs,
and other system-managed metadata.
- James
--
James Morris
<[email protected]>
On Thu, 2008-02-28 at 17:04 -0800, Casey Schaufler wrote:
> --- Dave Quigley <[email protected]> wrote:
>
> >
> > ...
> >
> > I can only speak for myself but honestly I've only seen Casey act
> > confrontational to this idea from the beginning.
>
> That is simply because I don't care for your design and implementation
> choices, I think they're a bad way to go, I've suggested what I
> think you should do, and I'm sorry that that comes off as
> confrontational but that does not change what I see as flaws in
> your approach. I understand what you're trying to do and I think
> it's wrong.
>
> > There is absolutely
> > nothing in here that is SELinux specific, tecnically its not even MAC
> > specific.
>
> Then why are you putting "mac" in the interface name?
>
> > I said from the beginning that this was perhaps not the best
> > name and we are willing to change it.
>
> If you read back in the thread, that is what I suggested you do.
I know but for some odd reason we kept arguing about it. Unless you want
me to repost the patch on it's own with the name changed you are going
to have to wait for version two :)
>
> > There is nothing in this hook that
> > wasn't in LSM before. This is almost identical functionality to what
> > Adrian removed in 2.6.24. The only difference between this and
> > security_inode_getsuffix is that this returns security.suffix and that
> > the name is different. I don't have a SMACK box to test it on but I'm
> > 99% sure that if Casey tried to use SMACK with this patch set that he
> > would have labeled nfs working with SMACK.
>
> You're very possibly right. I am not argueing from what's right for
> Smack, I am argueing from what's right for the LSM. Smack is a label
> based MAC LSM, like SELinux. I would expect that it would be easy for
> the NFS implementation to accomodate both.
>
> > If it doesn't work with SMACK
> > right now I'm willing to help him with that and even include it in the
> > patch set. But spreading FUD about how we are including SELinux specific
> > code in here is just that.
>
> Sorry, but I'm not argueing that it's SELinux specific at this point.
> I'm argueing that it's specific to single label stored in an xattr
> based MAC systems (a set of which both SELinux and Smack are members)
> and that it is file system specific to NFS. Any of these attributes
> makes it questionable as an LSM interface.
>
> As I said before, trying to be helpful, call it security_blob_name(),
> and the upcoming Discretionary Time Lock module can return NULL,
> indicating that it wants to share no blob name. Or call it
> security_xattr_names() and DTL can return NULL and B&L+Biba can
> return "security.Bell&LaPadula security.Biba", hoping that everyone
> who uses the interface accepts the blank seperation as an indication
> that there are multiple xattrs involved.
I agree with your suggestion here but nowhere in earlier emails did you
outline this. You just vaguely described a method that sounds like the
selinux sidtab. If you had described it this way in the beginning we
would have be done with after the first response. If we are going to
work well in the future you need to be more clear when you make
constructive criticisms (or even destructive ones *wink* ).
>
> I am saying that security_maclabel() is a bad choice, and I think
> that as an LSM (not MAC, not xattr, not NFS) interface it should
> serve the LSM, making the LSM interface better first, and being
> the specific interface that a particular file system finds
> convenient second.
>
> And before we go any further, I have personally been involved in
> doing labeled NFS three times, and I know where the bodies are
> buried. Your approach is fine for single label stored in xattr based
> MAC systems. It does not generalize worth catfish whiskers, whereas
> the two other schemes I've done do so flawlessly. I am critical of
> this approach only because I know that y'all can do better.
That is fine. I welcome constructive criticism but you have a tendency
of being vague with what you mean and at times it comes off the wrong
way. This is the whole reason the patch set was posted to begin with. We
have been working on it for so long without much outside input so we
decided to get criticism on it.
>
> Great. Now I owe the entire labeled NFS team beer.
>
> Thank you.
>
>
> Casey Schaufler
> [email protected]
--- Trond Myklebust <[email protected]> wrote:
>
> On Thu, 2008-02-28 at 19:39 -0500, Christoph Hellwig wrote:
> > On Thu, Feb 28, 2008 at 07:04:57PM -0500, Dave Quigley wrote:
> > > There are several things here. I've spoken to several people about this
> > > and the belief I've gotten from most of them is that a recommended
> > > attribute is how this is to be transported. The NFSv4 spec people will
> > > probably say that if you want xattr like functionality for NFSv4 use
> > > named attributes. For us this is not an option since we require
> > > semantics to label on create/open and the only way we can do this is by
> > > adding a recommended attribute. The create/open calls in NFSv4 takes a
> > > list of attributes to use on create as part of the request. I really
> > > don't see a difference between the security blob and the
> > > username/groupname that NFSv4 currently uses. Also there is a good
> > > chance that we will need to translate labels at some point (read future
> > > work).
> >
> > Then use the existing side-band protocol and ignore the NFSv4 spec
> > group. They're <skip colourful language here> anyway.
>
> As I've told you several times before: we're _NOT_ putting private
> ioctl^Hxattrs onto the wire. If the protocol can't be described in an
> RFC, then it isn't going in no matter what expletive you choose to
> use...
With the SGI supplied reference implementation it ought to be a
small matter of work to write an RFC. If the information weren't
SGI proprietary I could even tell you how long it ought to take
a junior engineer in Melbourne to write. The fact that there is
currently no RFC does not mean that there cannot be a RFC, only
that no one has written (or published) one yet.
Casey Schaufler
[email protected]
--- Dave Quigley <[email protected]> wrote:
>
> ...
>
> The reason we are trying to go through the standards process in the
> first place is that there is a desire to use SELinux with large netapp
> storage boxes. I don't believe that netapp supports the existing
> side-band protocol for NFSv4 and the impression I got was that they we
> were going to have an incredibly hard time convincing them of putting
> anything in that isn't part of the standard.
Most of the original SGI XFS team went to NetApp. The engineer
who developed the side-band xattr protocol (last I heard he was a
real estate speculator in Florida) spent lots of time with them.
They may not be so hostile to the idea as you seem to think.
> It seems that adding one
> recommended attribute which is described in a 3 page internet draft(Most
> of which is BS that is part of the template) would be significantly
> easier to get into the standard than trying to push an out of band xattr
> protocol.
Easier may be pragmatic, but that does not make it right.
I suggest, that in my opinion (there, is that sufficiently
non-confrontational?) that Linux and the LSM are much better
served by a general xattr protocol than by adding a single
reccommended attribute.
> Also, I believe Trond even expressed some discontent with it a
> while back when we first started development.
And he wasn't confrontational at all! (insert smiley here)
>
> ...
> >
> > And changing the name and minor details is exactly what Casey requested.
>
> I can always go with the original hook name of get_security_xattr_name
> which turns into a security_get_security_xattr_name call which seems a
> bit ludicrous.
Well, that's why I keep suggesting security_blob_name.
Do you have something against blobs?
> The only other complaint that I saw from Casey besides
> the name of the function was that there could be more than one xattr.
More precisely, I said that there could be a number other than one,
with zero also being an option, and the possibility existing that the
name of the blob might not be an xattr (it could be uid, gid, access
time, or inode number based) and still make a useful LSM.
> If
> we want to address that then I need another hook that says give me all
> data that the LSM deems important for this file. Which is essentially
> the same thing as taking each of the xattr names that the module will
> provide, grabbing each of them in turn, and concatenating them together.
That would be security_getblob(), would it not?
And if you have that, why do you need the attribute name?
> For SELinux this is no different than getsecurity with the selinux
> suffix. The same goes for SMACK.
True enough, but like I keep saying, those are both single label
stored in an xattr based MAC systems.
BTW, I prefer "Smack" to SMACK. Much less 1970's.
Thank you.
Casey Schaufler
[email protected]
--- Trond Myklebust <[email protected]> wrote:
>
> On Thu, 2008-02-28 at 19:51 -0500, Christoph Hellwig wrote:
> > On Thu, Feb 28, 2008 at 04:50:06PM -0800, Trond Myklebust wrote:
> > > As I've told you several times before: we're _NOT_ putting private
> > > ioctl^Hxattrs onto the wire. If the protocol can't be described in an
> > > RFC, then it isn't going in no matter what expletive you choose to
> > > use...
> >
> > It's as unstructured as the named attributes already in. Or file data
> > for that matter.
>
> Describing what is supposed to be a security mechanism in a structured
> fashion for use in a protocol should hardly be an impossible task (and
> AFAICS, Dave & co are making good progress in doing so). If it is, then
> that casts serious doubt on the validity of the security model...
Now this is were I always get confused. I sounds like you're
saying that a name/value pair is insufficiently structured for
use in a protocol specification.
> There should be no need for ioctls.
Sorry, but as far as I'm concerned you just threw a bunny under
the train for no apparent reason. What have ioctls got to do with
anything?
Casey Schaufler
[email protected]
On Thu, 2008-02-28 at 17:47 -0800, Casey Schaufler wrote:
> --- Dave Quigley <[email protected]> wrote:
>
> >
> > ...
> >
> > The reason we are trying to go through the standards process in the
> > first place is that there is a desire to use SELinux with large netapp
> > storage boxes. I don't believe that netapp supports the existing
> > side-band protocol for NFSv4 and the impression I got was that they we
> > were going to have an incredibly hard time convincing them of putting
> > anything in that isn't part of the standard.
>
> Most of the original SGI XFS team went to NetApp. The engineer
> who developed the side-band xattr protocol (last I heard he was a
> real estate speculator in Florida) spent lots of time with them.
> They may not be so hostile to the idea as you seem to think.
>
> > It seems that adding one
> > recommended attribute which is described in a 3 page internet draft(Most
> > of which is BS that is part of the template) would be significantly
> > easier to get into the standard than trying to push an out of band xattr
> > protocol.
>
> Easier may be pragmatic, but that does not make it right.
> I suggest, that in my opinion (there, is that sufficiently
> non-confrontational?) that Linux and the LSM are much better
> served by a general xattr protocol than by adding a single
> reccommended attribute.
You might be right that Linux and LSM are better served by this, but
this has to be used by more than just Linux. Solaris has the new FMAC
initiative (The F is silent) which will probably want to use this as
well. SEBSD/SEDarwin also has a use for this and they have a MAC label
concept in their OS with a system call for getting/setting it.
>
> > Also, I believe Trond even expressed some discontent with it a
> > while back when we first started development.
>
> And he wasn't confrontational at all! (insert smiley here)
I'm not about to get between him and Christoph :)
> >
> > ...
> > >
> > > And changing the name and minor details is exactly what Casey requested.
> >
> > I can always go with the original hook name of get_security_xattr_name
> > which turns into a security_get_security_xattr_name call which seems a
> > bit ludicrous.
>
> Well, that's why I keep suggesting security_blob_name.
> Do you have something against blobs?
>
> > The only other complaint that I saw from Casey besides
> > the name of the function was that there could be more than one xattr.
>
> More precisely, I said that there could be a number other than one,
> with zero also being an option, and the possibility existing that the
> name of the blob might not be an xattr (it could be uid, gid, access
> time, or inode number based) and still make a useful LSM.
It seems your argument is against using xattrs. Regardless of this hook
the 0 xattr LSM is still borked by this. security_inode_getsecurity(...,
suffix, ...). It is assumed that the fundamental function for getting
security information takes an xattr suffix. Don't bother responding to
this email eventually you will get to the one where I agree with you on
some points.
>
> > If
> > we want to address that then I need another hook that says give me all
> > data that the LSM deems important for this file. Which is essentially
> > the same thing as taking each of the xattr names that the module will
> > provide, grabbing each of them in turn, and concatenating them together.
>
> That would be security_getblob(), would it not?
> And if you have that, why do you need the attribute name?
Keep reading your emails :)
>
> > For SELinux this is no different than getsecurity with the selinux
> > suffix. The same goes for SMACK.
>
> True enough, but like I keep saying, those are both single label
> stored in an xattr based MAC systems.
>
> BTW, I prefer "Smack" to SMACK. Much less 1970's.
>
> Thank you.
>
>
> Casey Schaufler
> [email protected]
--- Dave Quigley <[email protected]> wrote:
>
> On Thu, 2008-02-28 at 20:00 -0500, Christoph Hellwig wrote:
> > On Thu, Feb 28, 2008 at 07:32:47PM -0500, Dave Quigley wrote:
> > > I can always go with the original hook name of get_security_xattr_name
> > > which turns into a security_get_security_xattr_name call which seems a
> > > bit ludicrous. The only other complaint that I saw from Casey besides
> > > the name of the function was that there could be more than one xattr. If
> > > we want to address that then I need another hook that says give me all
> > > data that the LSM deems important for this file. Which is essentially
> > > the same thing as taking each of the xattr names that the module will
> > > provide, grabbing each of them in turn, and concatenating them together.
> > > For SELinux this is no different than getsecurity with the selinux
> > > suffix. The same goes for SMACK.
> >
> > What about Casey's suggestion of get_security_blob? For any reasonable
> > module that just has a single xattr it's trivial and for those that
> > have multiple or a different storage model it might get complicated
> > but that's not our problem for now.
>
> If this is the method we are going to use then we need a corresponding
> set_security_blob as well.
Not to sound stupid, but why would you need this?
> This seems like a paradigm shift for
> accessing security information in the kernel.
Well, yes, but look at David Howell's file cacheing work
before you take too firm a stand.
> I said to Casey in the
> beginning that I'd be willing to revisit it but that neither he or I
> alone could make the decision. Unless I misunderstood the original
> mandate for security information and that it only applies to how user
> space accesses it.
Sorry, I don't understand how user space and mandates go together here.
Casey Schaufler
[email protected]
On Thu, 2008-02-28 at 18:07 -0800, Casey Schaufler wrote:
> --- Dave Quigley <[email protected]> wrote:
>
> >
> > On Thu, 2008-02-28 at 20:00 -0500, Christoph Hellwig wrote:
> > > On Thu, Feb 28, 2008 at 07:32:47PM -0500, Dave Quigley wrote:
> > > > I can always go with the original hook name of get_security_xattr_name
> > > > which turns into a security_get_security_xattr_name call which seems a
> > > > bit ludicrous. The only other complaint that I saw from Casey besides
> > > > the name of the function was that there could be more than one xattr. If
> > > > we want to address that then I need another hook that says give me all
> > > > data that the LSM deems important for this file. Which is essentially
> > > > the same thing as taking each of the xattr names that the module will
> > > > provide, grabbing each of them in turn, and concatenating them together.
> > > > For SELinux this is no different than getsecurity with the selinux
> > > > suffix. The same goes for SMACK.
> > >
> > > What about Casey's suggestion of get_security_blob? For any reasonable
> > > module that just has a single xattr it's trivial and for those that
> > > have multiple or a different storage model it might get complicated
> > > but that's not our problem for now.
> >
> > If this is the method we are going to use then we need a corresponding
> > set_security_blob as well.
>
> Not to sound stupid, but why would you need this?
What do you intend to do with this blob once you have it? Somehow it
needs to be set on the other end. So unless you want each LSM
decomposing the blob inside of NFS you need a hook to allow it to do so.
>
> > This seems like a paradigm shift for
> > accessing security information in the kernel.
>
> Well, yes, but look at David Howell's file cacheing work
> before you take too firm a stand.
>
> > I said to Casey in the
> > beginning that I'd be willing to revisit it but that neither he or I
> > alone could make the decision. Unless I misunderstood the original
> > mandate for security information and that it only applies to how user
> > space accesses it.
>
> Sorry, I don't understand how user space and mandates go together here
I was inquiring if the mandate to use xattrs for security attributes was
only for userspace's access to them and the kernel could create separate
interfaces for it.
> .
>
>
> Casey Schaufler
> [email protected]
On Thu, 28 Feb 2008, Casey Schaufler wrote:
> Easier may be pragmatic, but that does not make it right.
> I suggest, that in my opinion (there, is that sufficiently
> non-confrontational?) that Linux and the LSM are much better
> served by a general xattr protocol than by adding a single
> reccommended attribute.
An xattr protocol is overkill for conveying a MAC label over the network,
and would still not provide the required semantics.
Please see prior discussion on this e.g.
http://marc.info/?l=linux-kernel&m=120424789929258&w=2
Note that RAs are already used to convey ACLs and all other system-managed
metatdata. i.e. an extensible, appropriate infrastructure already exists
in the NFSv4 protocol, and has been used successfully for similar
purposes. We do not need to add a new, generalized protocol to NFSv4
for this, especially one which does not meet the requirements.
- James
--
James Morris
<[email protected]>
--- Dave Quigley <[email protected]> wrote:
>
> On Thu, 2008-02-28 at 17:04 -0800, Casey Schaufler wrote:
> > --- Dave Quigley <[email protected]> wrote:
> >
> > >
> > > ...
> > >
> > > I can only speak for myself but honestly I've only seen Casey act
> > > confrontational to this idea from the beginning.
> >
> > That is simply because I don't care for your design and implementation
> > choices, I think they're a bad way to go, I've suggested what I
> > think you should do, and I'm sorry that that comes off as
> > confrontational but that does not change what I see as flaws in
> > your approach. I understand what you're trying to do and I think
> > it's wrong.
> >
> > > There is absolutely
> > > nothing in here that is SELinux specific, tecnically its not even MAC
> > > specific.
> >
> > Then why are you putting "mac" in the interface name?
> >
> > > I said from the beginning that this was perhaps not the best
> > > name and we are willing to change it.
> >
> > If you read back in the thread, that is what I suggested you do.
>
> I know but for some odd reason we kept arguing about it. Unless you want
> me to repost the patch on it's own with the name changed you are going
> to have to wait for version two :)
No trouble there.
> >
> > > There is nothing in this hook that
> > > wasn't in LSM before. This is almost identical functionality to what
> > > Adrian removed in 2.6.24. The only difference between this and
> > > security_inode_getsuffix is that this returns security.suffix and that
> > > the name is different. I don't have a SMACK box to test it on but I'm
> > > 99% sure that if Casey tried to use SMACK with this patch set that he
> > > would have labeled nfs working with SMACK.
> >
> > You're very possibly right. I am not argueing from what's right for
> > Smack, I am argueing from what's right for the LSM. Smack is a label
> > based MAC LSM, like SELinux. I would expect that it would be easy for
> > the NFS implementation to accomodate both.
> >
> > > If it doesn't work with SMACK
> > > right now I'm willing to help him with that and even include it in the
> > > patch set. But spreading FUD about how we are including SELinux specific
> > > code in here is just that.
> >
> > Sorry, but I'm not argueing that it's SELinux specific at this point.
> > I'm argueing that it's specific to single label stored in an xattr
> > based MAC systems (a set of which both SELinux and Smack are members)
> > and that it is file system specific to NFS. Any of these attributes
> > makes it questionable as an LSM interface.
> >
> > As I said before, trying to be helpful, call it security_blob_name(),
> > and the upcoming Discretionary Time Lock module can return NULL,
> > indicating that it wants to share no blob name. Or call it
> > security_xattr_names() and DTL can return NULL and B&L+Biba can
> > return "security.Bell&LaPadula security.Biba", hoping that everyone
> > who uses the interface accepts the blank seperation as an indication
> > that there are multiple xattrs involved.
>
> I agree with your suggestion here but nowhere in earlier emails did you
> outline this. You just vaguely described a method that sounds like the
> selinux sidtab. If you had described it this way in the beginning we
> would have be done with after the first response. If we are going to
> work well in the future you need to be more clear when you make
> constructive criticisms (or even destructive ones *wink* ).
Sorry 'bout that.
> >
> > I am saying that security_maclabel() is a bad choice, and I think
> > that as an LSM (not MAC, not xattr, not NFS) interface it should
> > serve the LSM, making the LSM interface better first, and being
> > the specific interface that a particular file system finds
> > convenient second.
> >
> > And before we go any further, I have personally been involved in
> > doing labeled NFS three times, and I know where the bodies are
> > buried. Your approach is fine for single label stored in xattr based
> > MAC systems. It does not generalize worth catfish whiskers, whereas
> > the two other schemes I've done do so flawlessly. I am critical of
> > this approach only because I know that y'all can do better.
>
> That is fine. I welcome constructive criticism but you have a tendency
> of being vague with what you mean and at times it comes off the wrong
> way. This is the whole reason the patch set was posted to begin with. We
> have been working on it for so long without much outside input so we
> decided to get criticism on it.
Thank you for doing so.
> > Great. Now I owe the entire labeled NFS team beer.
Phew, he missed that one.
Casey Schaufler
[email protected]
On Thu, 2008-02-28 at 18:29 -0800, Casey Schaufler wrote:
> --- Dave Quigley <[email protected]> wrote:
>
> >
> > On Thu, 2008-02-28 at 17:04 -0800, Casey Schaufler wrote:
> > > --- Dave Quigley <[email protected]> wrote:
> > >
> > > >
> > > > ...
> > > >
> > > > I can only speak for myself but honestly I've only seen Casey act
> > > > confrontational to this idea from the beginning.
> > >
> > > That is simply because I don't care for your design and implementation
> > > choices, I think they're a bad way to go, I've suggested what I
> > > think you should do, and I'm sorry that that comes off as
> > > confrontational but that does not change what I see as flaws in
> > > your approach. I understand what you're trying to do and I think
> > > it's wrong.
> > >
> > > > There is absolutely
> > > > nothing in here that is SELinux specific, tecnically its not even MAC
> > > > specific.
> > >
> > > Then why are you putting "mac" in the interface name?
> > >
> > > > I said from the beginning that this was perhaps not the best
> > > > name and we are willing to change it.
> > >
> > > If you read back in the thread, that is what I suggested you do.
> >
> > I know but for some odd reason we kept arguing about it. Unless you want
> > me to repost the patch on it's own with the name changed you are going
> > to have to wait for version two :)
>
> No trouble there.
>
> > >
> > > > There is nothing in this hook that
> > > > wasn't in LSM before. This is almost identical functionality to what
> > > > Adrian removed in 2.6.24. The only difference between this and
> > > > security_inode_getsuffix is that this returns security.suffix and that
> > > > the name is different. I don't have a SMACK box to test it on but I'm
> > > > 99% sure that if Casey tried to use SMACK with this patch set that he
> > > > would have labeled nfs working with SMACK.
> > >
> > > You're very possibly right. I am not argueing from what's right for
> > > Smack, I am argueing from what's right for the LSM. Smack is a label
> > > based MAC LSM, like SELinux. I would expect that it would be easy for
> > > the NFS implementation to accomodate both.
> > >
> > > > If it doesn't work with SMACK
> > > > right now I'm willing to help him with that and even include it in the
> > > > patch set. But spreading FUD about how we are including SELinux specific
> > > > code in here is just that.
> > >
> > > Sorry, but I'm not argueing that it's SELinux specific at this point.
> > > I'm argueing that it's specific to single label stored in an xattr
> > > based MAC systems (a set of which both SELinux and Smack are members)
> > > and that it is file system specific to NFS. Any of these attributes
> > > makes it questionable as an LSM interface.
> > >
> > > As I said before, trying to be helpful, call it security_blob_name(),
> > > and the upcoming Discretionary Time Lock module can return NULL,
> > > indicating that it wants to share no blob name. Or call it
> > > security_xattr_names() and DTL can return NULL and B&L+Biba can
> > > return "security.Bell&LaPadula security.Biba", hoping that everyone
> > > who uses the interface accepts the blank seperation as an indication
> > > that there are multiple xattrs involved.
> >
> > I agree with your suggestion here but nowhere in earlier emails did you
> > outline this. You just vaguely described a method that sounds like the
> > selinux sidtab. If you had described it this way in the beginning we
> > would have be done with after the first response. If we are going to
> > work well in the future you need to be more clear when you make
> > constructive criticisms (or even destructive ones *wink* ).
>
> Sorry 'bout that.
>
> > >
> > > I am saying that security_maclabel() is a bad choice, and I think
> > > that as an LSM (not MAC, not xattr, not NFS) interface it should
> > > serve the LSM, making the LSM interface better first, and being
> > > the specific interface that a particular file system finds
> > > convenient second.
> > >
> > > And before we go any further, I have personally been involved in
> > > doing labeled NFS three times, and I know where the bodies are
> > > buried. Your approach is fine for single label stored in xattr based
> > > MAC systems. It does not generalize worth catfish whiskers, whereas
> > > the two other schemes I've done do so flawlessly. I am critical of
> > > this approach only because I know that y'all can do better.
> >
> > That is fine. I welcome constructive criticism but you have a tendency
> > of being vague with what you mean and at times it comes off the wrong
> > way. This is the whole reason the patch set was posted to begin with. We
> > have been working on it for so long without much outside input so we
> > decided to get criticism on it.
>
> Thank you for doing so.
>
> > > Great. Now I owe the entire labeled NFS team beer.
>
> Phew, he missed that one.
Hehe I didn't miss it but I don't drink (A coke would be greatly
appreciated though). Can't speak for the rest of the team though.
>
>
> Casey Schaufler
> [email protected]
On Thu, 2008-02-28 at 17:26 -0800, Casey Schaufler wrote:
> --- Trond Myklebust <[email protected]> wrote:
>
> >
> > On Thu, 2008-02-28 at 19:39 -0500, Christoph Hellwig wrote:
> > > On Thu, Feb 28, 2008 at 07:04:57PM -0500, Dave Quigley wrote:
> > > > There are several things here. I've spoken to several people about this
> > > > and the belief I've gotten from most of them is that a recommended
> > > > attribute is how this is to be transported. The NFSv4 spec people will
> > > > probably say that if you want xattr like functionality for NFSv4 use
> > > > named attributes. For us this is not an option since we require
> > > > semantics to label on create/open and the only way we can do this is by
> > > > adding a recommended attribute. The create/open calls in NFSv4 takes a
> > > > list of attributes to use on create as part of the request. I really
> > > > don't see a difference between the security blob and the
> > > > username/groupname that NFSv4 currently uses. Also there is a good
> > > > chance that we will need to translate labels at some point (read future
> > > > work).
> > >
> > > Then use the existing side-band protocol and ignore the NFSv4 spec
> > > group. They're <skip colourful language here> anyway.
> >
> > As I've told you several times before: we're _NOT_ putting private
> > ioctl^Hxattrs onto the wire. If the protocol can't be described in an
> > RFC, then it isn't going in no matter what expletive you choose to
> > use...
>
> With the SGI supplied reference implementation it ought to be a
> small matter of work to write an RFC. If the information weren't
> SGI proprietary I could even tell you how long it ought to take
> a junior engineer in Melbourne to write. The fact that there is
> currently no RFC does not mean that there cannot be a RFC, only
> that no one has written (or published) one yet.
NO! It is not a "small matter of work".
The fact is that private crap like the 'security' and 'system' namespace
makes describing 'xattr' as a protocol a non-starter and an
interoperability nightmare. If you can't do better than xattr for a
security protocol, then it is time to go look for another job...
On Thu, 2008-02-28 at 17:55 -0800, Casey Schaufler wrote:
> > There should be no need for ioctls.
>
> Sorry, but as far as I'm concerned you just threw a bunny under
> the train for no apparent reason. What have ioctls got to do with
> anything?
What part of 'interoperability' don't you get here?
There is no room for extensions that allow clients+servers to establish
arbitrary private protocols.
On Wed, 27 Feb 2008 15:39:38 -0500 "David P. Quigley" <[email protected]> wrote:
> +#ifdef CONFIG_SECURITY
> + if (ia_valid & ATTR_SECURITY_LABEL) {
> + char *key = (char *)security_maclabel_getname();
> + vfs_setxattr_locked(dentry, key,
> + attr->ia_label, attr->ia_label_len, 0);
It would be nicer to change vfs_setxattr_locked() so that it
takes a const char *.
On Thu, 2008-02-28 at 20:00 -0500, Christoph Hellwig wrote:
> On Thu, Feb 28, 2008 at 07:32:47PM -0500, Dave Quigley wrote:
> > I can always go with the original hook name of get_security_xattr_name
> > which turns into a security_get_security_xattr_name call which seems a
> > bit ludicrous. The only other complaint that I saw from Casey besides
> > the name of the function was that there could be more than one xattr. If
> > we want to address that then I need another hook that says give me all
> > data that the LSM deems important for this file. Which is essentially
> > the same thing as taking each of the xattr names that the module will
> > provide, grabbing each of them in turn, and concatenating them together.
> > For SELinux this is no different than getsecurity with the selinux
> > suffix. The same goes for SMACK.
>
> What about Casey's suggestion of get_security_blob? For any reasonable
> module that just has a single xattr it's trivial and for those that
> have multiple or a different storage model it might get complicated
> but that's not our problem for now.
Possibly I'm missing something, but if I'm implementing a security
module that has any security attribute at all, e.g. capability module
with security.capability, and I see a hook called "get_security_blob" or
"get_security_attr" or the like, I'll implement that hook and return my
attribute there. Which in turn will _break_ the labeled NFS
functionality because it is expecting a MAC label specifically.
The whole point here is that we do not want modules like capability to
return their security attributes here, because this is to support
labeled NFS functionality in support of enforcing MAC.
I don't especially care about the hook name per se, but the interface
(whatever it may be) needs to convey the proper semantics, and the
semantics truly are MAC specific (and should be).
--
Stephen Smalley
National Security Agency
On Thu, 2008-02-28 at 18:48 -0500, Christoph Hellwig wrote:
> On Thu, Feb 28, 2008 at 02:30:35PM -0500, Stephen Smalley wrote:
> > This is an interface to be used by NFS to get information from the
> > security module. The information desired is specific to the MAC
> > labeling functionality in NFSv4 that is being proposed. That
> > functionality is MAC specific (necessarily so, just like the ACL
> > functionality is ACL specific). We are hiding the SELinux-specific bits
> > behind the LSM interface, and non-MAC LSMs are free to return NULL in
> > order to indicate that they don't support MAC labeling. We do NOT want
> > the capability module to return its security blob here, or any other
> > non-MAC LSM - it will yield the wrong semantics for the NFS MAC support.
>
> I think Casey is totally right here. The LSM interface should not be
> as specific here. If you want to limit the NFSv4 interface to single
> MAC xattr label based systems add an additional method to check if
> the LSM is that. But the proper fix is of course to not add somthing
> so specific to NFSv4 at all, as it's got enough shortcoming already.
> Please add a proper xattr protocol. It's not like it's hard, SGI
> has been doing this in IRIX for NFSv3 for ages as a sideband protocol,
> and even release the reference source under the GPL. Just either use
> that with NFSv4 or if you feel fancy merge it into the NFS spec for
> NFSv6^H4.2.
>
> > In any event, I don't think we need your permission.
>
> Wow, that's rude even to someone as direct as me. Casey is the only
> other person having an in-tree LSM, and I think his input in this
> area is important. But if not I as a VFS person can happily give
> you my "no" for the current version from the VFS point of view.
Fair point - my apologies to Casey.
--
Stephen Smalley
National Security Agency
On Fri, 2008-02-29 at 08:30 -0500, Stephen Smalley wrote:
> On Thu, 2008-02-28 at 20:00 -0500, Christoph Hellwig wrote:
> > On Thu, Feb 28, 2008 at 07:32:47PM -0500, Dave Quigley wrote:
> > > I can always go with the original hook name of get_security_xattr_name
> > > which turns into a security_get_security_xattr_name call which seems a
> > > bit ludicrous. The only other complaint that I saw from Casey besides
> > > the name of the function was that there could be more than one xattr. If
> > > we want to address that then I need another hook that says give me all
> > > data that the LSM deems important for this file. Which is essentially
> > > the same thing as taking each of the xattr names that the module will
> > > provide, grabbing each of them in turn, and concatenating them together.
> > > For SELinux this is no different than getsecurity with the selinux
> > > suffix. The same goes for SMACK.
> >
> > What about Casey's suggestion of get_security_blob? For any reasonable
> > module that just has a single xattr it's trivial and for those that
> > have multiple or a different storage model it might get complicated
> > but that's not our problem for now.
>
> Possibly I'm missing something, but if I'm implementing a security
> module that has any security attribute at all, e.g. capability module
> with security.capability, and I see a hook called "get_security_blob" or
> "get_security_attr" or the like, I'll implement that hook and return my
> attribute there. Which in turn will _break_ the labeled NFS
> functionality because it is expecting a MAC label specifically.
>
> The whole point here is that we do not want modules like capability to
> return their security attributes here, because this is to support
> labeled NFS functionality in support of enforcing MAC.
>
> I don't especially care about the hook name per se, but the interface
> (whatever it may be) needs to convey the proper semantics, and the
> semantics truly are MAC specific (and should be).
BTW, to date, "security blob" has been used to refer to the structures
allocated by the security modules referenced by the void* security
fields in the kernel data structures (task, inode, ...), while
"secctx" (security context) and "secid" (security id) have been
leveraged by subsystems like audit, netlink and labeled IPSEC to
represent security labels.
--
Stephen Smalley
National Security Agency
--- Trond Myklebust <[email protected]> wrote:
>
> ...
> > With the SGI supplied reference implementation it ought to be a
> > small matter of work to write an RFC. If the information weren't
> > SGI proprietary I could even tell you how long it ought to take
> > a junior engineer in Melbourne to write. The fact that there is
> > currently no RFC does not mean that there cannot be a RFC, only
> > that no one has written (or published) one yet.
>
> NO! It is not a "small matter of work".
Ah, well, I don't understand why, but that's probably just
me being ignorant. It happens from time to time.
> The fact is that private crap like the 'security' and 'system' namespace
> makes describing 'xattr' as a protocol a non-starter and an
> interoperability nightmare.
Ok, I can buy that it doesn't fit in with the current protocol
mindset, and that I for one have not demonstrated that it can
be. I remember how upset the IETF got over the original CIPSO
proposal not specifying which label tag value coresponded to
"Top Secret".
> If you can't do better than xattr for a
> security protocol, then it is time to go look for another job...
But ... I don't have a job. You're being mean. (smiley)
I think that we have a conflict between what works well for
a filesystem (xattrs are really helpful) and what works well
for a network protocol (undefined blobs of goo are atrocious)
in the case of a network file system. From either standpoint
the other is completely unworkable.
It may be the case that for NFS the proposed scheme (delta
LSM naming propriety, which is getting addressed) is the
best we can do. NFS is an old protocol (older than some of
the people reading this) and should be excused some shortcomings.
Thank you.
Casey Schaufler
[email protected]
--- Trond Myklebust <[email protected]> wrote:
>
> On Thu, 2008-02-28 at 17:55 -0800, Casey Schaufler wrote:
>
> > > There should be no need for ioctls.
> >
> > Sorry, but as far as I'm concerned you just threw a bunny under
> > the train for no apparent reason. What have ioctls got to do with
> > anything?
>
> What part of 'interoperability' don't you get here?
I can understand that an implementation of NFS+xattr could
present some issues where only one side speaks the xattr
protocols, or where they speak them differently. If that's
a showstopper from the network protocol side (it isn't from
the file system end, client or server) then you're right,
it won't work. What I don't understand is why it would be
a showstopper. The current NFS protocol makes all sorts of
assumptions about the data it passes (like the uids on the
two ends mapping sanely) that seem to me to be as much a
problem as xattr value mapping.
> There is no room for extensions that allow clients+servers to establish
> arbitrary private protocols.
I think that I understand that you believe that, what I
don't understand is why you believe that. Is it an artifact
of the locking protocol that only worked about half the
time, and then in a half donkied way?
I'm not trying to be an obstructionist idiot here, even if
that's how it may appear. I have my pet solution, I really
don't see the problem with it, and it looks to me like the
arguements against it are either so obvious I'm missing them
(does happen) or based on dogmas I don't subscribe to.
Thank you for helping me (and maybe some others) understand
the issues we're up against so that I (we) can address issues
better in the future.
Casey Schaufler
[email protected]
--- Stephen Smalley <[email protected]> wrote:
>
> On Thu, 2008-02-28 at 18:48 -0500, Christoph Hellwig wrote:
> > On Thu, Feb 28, 2008 at 02:30:35PM -0500, Stephen Smalley wrote:
> > > This is an interface to be used by NFS to get information from the
> > > security module. The information desired is specific to the MAC
> > > labeling functionality in NFSv4 that is being proposed. That
> > > functionality is MAC specific (necessarily so, just like the ACL
> > > functionality is ACL specific). We are hiding the SELinux-specific bits
> > > behind the LSM interface, and non-MAC LSMs are free to return NULL in
> > > order to indicate that they don't support MAC labeling. We do NOT want
> > > the capability module to return its security blob here, or any other
> > > non-MAC LSM - it will yield the wrong semantics for the NFS MAC support.
> >
> > I think Casey is totally right here. The LSM interface should not be
> > as specific here. If you want to limit the NFSv4 interface to single
> > MAC xattr label based systems add an additional method to check if
> > the LSM is that. But the proper fix is of course to not add somthing
> > so specific to NFSv4 at all, as it's got enough shortcoming already.
> > Please add a proper xattr protocol. It's not like it's hard, SGI
> > has been doing this in IRIX for NFSv3 for ages as a sideband protocol,
> > and even release the reference source under the GPL. Just either use
> > that with NFSv4 or if you feel fancy merge it into the NFS spec for
> > NFSv6^H4.2.
> >
> > > In any event, I don't think we need your permission.
> >
> > Wow, that's rude even to someone as direct as me. Casey is the only
> > other person having an in-tree LSM, and I think his input in this
> > area is important. But if not I as a VFS person can happily give
> > you my "no" for the current version from the VFS point of view.
>
> Fair point - my apologies to Casey.
Accepted. Now we work together like horses in troika.
Casey Schaufler
[email protected]
On Fri, 2008-02-29 at 09:46 -0800, Casey Schaufler wrote:
> > There is no room for extensions that allow clients+servers to establish
> > arbitrary private protocols.
>
> I think that I understand that you believe that, what I
> don't understand is why you believe that. Is it an artifact
> of the locking protocol that only worked about half the
> time, and then in a half donkied way?
>
> I'm not trying to be an obstructionist idiot here, even if
> that's how it may appear. I have my pet solution, I really
> don't see the problem with it, and it looks to me like the
> arguements against it are either so obvious I'm missing them
> (does happen) or based on dogmas I don't subscribe to.
>
> Thank you for helping me (and maybe some others) understand
> the issues we're up against so that I (we) can address issues
> better in the future.
Two of the main reasons for NFS's success as a protocol are the facts
that it is (more or less) standardized, while remaining (again more or
less) back-end agnostic. I can take pretty much any client from any one
vendor and any server from any other vendor, and make them work
together.
The reason why this works is mainly because the protocol has built upon
a consensus assumption of POSIX filesystem semantics on the servers
(hence, BTW, the pain when the IETF requested that we add
Microsoft-compatible semantics to NFSv4 as a precondition for making it
a standard).
If you look back at the NFS extensions that failed, and fell by the
road, then they tend to be the semi-private non-posix extensions
(typically ACL semantics, xattrs/named attributes, "secure NFS",...)
which break the underlying assumption that I can mix and match clients
and servers.
<rhetorical>Does that mean that we shouldn't provide extensions
protocols for doing these things?</rhetorical> Of course not... The
point about such extensions is that they need to be agreed upon by the
NFS community/stakeholders, in much the same way that any changes to the
kernel need to be agreed upon by the Linux community/stakeholders.
Adding a mechanism that allows subsets of clients/servers to set up
private protocols circumvents that consensus process, and are therefore
a bad thing, and should be avoided. That would be engaging in the exact
same "embrace, extend and extinguish" tactics for which we keep
criticizing certain other monopolists.
This should be a no-brainer...
Trond
--- Trond Myklebust <[email protected]> wrote:
> On Fri, 2008-02-29 at 09:46 -0800, Casey Schaufler wrote:
> > > There is no room for extensions that allow clients+servers to establish
> > > arbitrary private protocols.
> >
> > I think that I understand that you believe that, what I
> > don't understand is why you believe that. Is it an artifact
> > of the locking protocol that only worked about half the
> > time, and then in a half donkied way?
> >
> > I'm not trying to be an obstructionist idiot here, even if
> > that's how it may appear. I have my pet solution, I really
> > don't see the problem with it, and it looks to me like the
> > arguements against it are either so obvious I'm missing them
> > (does happen) or based on dogmas I don't subscribe to.
> >
> > Thank you for helping me (and maybe some others) understand
> > the issues we're up against so that I (we) can address issues
> > better in the future.
>
> Two of the main reasons for NFS's success as a protocol are the facts
> that it is (more or less) standardized, while remaining (again more or
> less) back-end agnostic. I can take pretty much any client from any one
> vendor and any server from any other vendor, and make them work
> together.
>
> The reason why this works is mainly because the protocol has built upon
> a consensus assumption of POSIX filesystem semantics on the servers
> (hence, BTW, the pain when the IETF requested that we add
> Microsoft-compatible semantics to NFSv4 as a precondition for making it
> a standard).
Ok, and since there is no POSIX file system semantic defined
for extended attributes it's really tough to create a protocol
specification that implements the POSIX file system semantics.
> If you look back at the NFS extensions that failed, and fell by the
> road, then they tend to be the semi-private non-posix extensions
> (typically ACL semantics, xattrs/named attributes, "secure NFS",...)
> which break the underlying assumption that I can mix and match clients
> and servers.
And without a definition of what behavior should be on the file system
you can't really say what the behavior should be in the network
protocol in the case where one end does not support the behavior.
> <rhetorical>Does that mean that we shouldn't provide extensions
> protocols for doing these things?</rhetorical> Of course not... The
> point about such extensions is that they need to be agreed upon by the
> NFS community/stakeholders, in much the same way that any changes to the
> kernel need to be agreed upon by the Linux community/stakeholders.
And a precursor to this is that the community agree on the underlying
file system semantics. Just because xattrs work on Irix and Linux
doesn't make them standard, and it would be rough going to claim
that the existance of those two implementations indicates stakeholder
acceptance.
> Adding a mechanism that allows subsets of clients/servers to set up
> private protocols circumvents that consensus process, and are therefore
> a bad thing, and should be avoided. That would be engaging in the exact
> same "embrace, extend and extinguish" tactics for which we keep
> criticizing certain other monopolists.
So it sounds as if for an xattr protocol to be viable it would first
require that xattr semantics be generally accepted (POSIX definition
would suffice), that there be multiple implementations (Linux and Irix
could suffice should Irix still be around when POSIX is done), and
that there be a perceived need beyond that of the Lunitic Fringe
Security Community.
> This should be a no-brainer...
I hope that I've wrapped my brain around your rationale.
If I have missed the point again, don't hesitate to correct me.
Thank you.
Casey Schaufler
[email protected]
On Fri, 2008-02-29 at 10:52 -0800, Casey Schaufler wrote:
> So it sounds as if for an xattr protocol to be viable it would first
> require that xattr semantics be generally accepted (POSIX definition
> would suffice), that there be multiple implementations (Linux and Irix
> could suffice should Irix still be around when POSIX is done), and
> that there be a perceived need beyond that of the Lunitic Fringe
> Security Community.
The problem isn't that of supporting the naive user xattr model: we can
almost do that within the existing 'named attribute' model of NFSv4. The
problem is that of supporting the arbitrary "security metadata" that are
allowed to have side-effects on the system behaviour, and that we appear
to have thought was a good idea to overload onto the xattr interface.
In the case of maclabels, where the "side-effect" is to describe and
enable extra access control rules, then you have the potential for
setting people up with a major interoperability problem. Using a
dedicated interface for it instead of overloading a Linux-style xattr
interface allows you to limit the scope of the documentation problem
that you would otherwise have.
Trond
--- Trond Myklebust <[email protected]> wrote:
> On Fri, 2008-02-29 at 10:52 -0800, Casey Schaufler wrote:
> > So it sounds as if for an xattr protocol to be viable it would first
> > require that xattr semantics be generally accepted (POSIX definition
> > would suffice), that there be multiple implementations (Linux and Irix
> > could suffice should Irix still be around when POSIX is done), and
> > that there be a perceived need beyond that of the Lunitic Fringe
> > Security Community.
>
> The problem isn't that of supporting the naive user xattr model: we can
> almost do that within the existing 'named attribute' model of NFSv4. The
> problem is that of supporting the arbitrary "security metadata" that are
> allowed to have side-effects on the system behaviour, and that we appear
> to have thought was a good idea to overload onto the xattr interface.
Hum. Security metadata was one of the justifications for the
original implementation of the xattr interface for XFS at SGI.
The implementation was intended to be generic and allow for
storage of data that impacts system behavior. No, it is not
overloading at all, it is really supposed to be used that way.
That's how it works on CXFS, which I know is still proprietary,
but which could become an open peer of NFS someday.
> In the case of maclabels, where the "side-effect" is to describe and
> enable extra access control rules, then you have the potential for
> setting people up with a major interoperability problem. Using a
> dedicated interface for it instead of overloading a Linux-style xattr
> interface allows you to limit the scope of the documentation problem
> that you would otherwise have.
Yes, I can see that having a specific interface reduces the
documentation required, and simplifies it as well. Unfortunately,
given the way that a secctx is defined for either SELinux or
Smack, and the fact that the relationships between secctx values
are defined independently on the server and client* it does not
appear that the interoperability issue has been addressed, or
even really acknowleged with the proposed scheme. Yes, the issue
of label translation has been acknowleged, but it appears to me
that a day one solution is required for the scheme to be useful.
So I suggest, again from a position of possible ignorance, that
the proposed scheme suffers from some of the same interoperability
and specification issues that a name/value pair scheme does, with
the only real improvement being that the name part is hard coded.
Perhaps that is sufficient improvement to justify the loss of
generality, but I personally wouldn't think so.
-----
* Identical SELinux policy or Smack rule specifications are not
necessaily sufficient to ensure label transparency.
Casey Schaufler
[email protected]
On Fri, 2008-02-29 at 13:07 -0800, Casey Schaufler wrote:
> --- Trond Myklebust <[email protected]> wrote:
>
> > On Fri, 2008-02-29 at 10:52 -0800, Casey Schaufler wrote:
> > > So it sounds as if for an xattr protocol to be viable it would first
> > > require that xattr semantics be generally accepted (POSIX definition
> > > would suffice), that there be multiple implementations (Linux and Irix
> > > could suffice should Irix still be around when POSIX is done), and
> > > that there be a perceived need beyond that of the Lunitic Fringe
> > > Security Community.
> >
> > The problem isn't that of supporting the naive user xattr model: we can
> > almost do that within the existing 'named attribute' model of NFSv4. The
> > problem is that of supporting the arbitrary "security metadata" that are
> > allowed to have side-effects on the system behaviour, and that we appear
> > to have thought was a good idea to overload onto the xattr interface.
>
> Hum. Security metadata was one of the justifications for the
> original implementation of the xattr interface for XFS at SGI.
> The implementation was intended to be generic and allow for
> storage of data that impacts system behavior. No, it is not
> overloading at all, it is really supposed to be used that way.
> That's how it works on CXFS, which I know is still proprietary,
> but which could become an open peer of NFS someday.
>
> > In the case of maclabels, where the "side-effect" is to describe and
> > enable extra access control rules, then you have the potential for
> > setting people up with a major interoperability problem. Using a
> > dedicated interface for it instead of overloading a Linux-style xattr
> > interface allows you to limit the scope of the documentation problem
> > that you would otherwise have.
>
> Yes, I can see that having a specific interface reduces the
> documentation required, and simplifies it as well. Unfortunately,
> given the way that a secctx is defined for either SELinux or
> Smack, and the fact that the relationships between secctx values
> are defined independently on the server and client* it does not
> appear that the interoperability issue has been addressed, or
> even really acknowleged with the proposed scheme. Yes, the issue
> of label translation has been acknowleged, but it appears to me
> that a day one solution is required for the scheme to be useful.
I completely disagree here. The Linux development model isn't to code
the entire thing throw it over a wall and then deal with the collateral
damage. This first version assumes a heterogenous environment and from
what we see so far that seems to be the common usecase for this
technology. A prototype implementation is already done for label
translations and it does need to be outlined in the RFC (Which I've
already started doing). However it is not necessary for an initial
release. The translation engine allows you to plug in an arbitrary
module to support whatever LSM you are going to use so this end of the
architecture is agnostic to the format that is going to be used on the
wire. For now that format is just a secctx which assumes the LSM running
on both ends is the same. Once the basics are refined and we can use it
as a base we will keep adding more functionality (process label
transport, better change notification, server side policy enforcement,
translation mappings.)
This is just a tiny fraction of what James outlined in the requirements
document. So, one step at a time lest we trip over imaginary stones.
>
> So I suggest, again from a position of possible ignorance, that
> the proposed scheme suffers from some of the same interoperability
> and specification issues that a name/value pair scheme does, with
> the only real improvement being that the name part is hard coded.
> Perhaps that is sufficient improvement to justify the loss of
> generality, but I personally wouldn't think so.
>
> -----
> * Identical SELinux policy or Smack rule specifications are not
> necessaily sufficient to ensure label transparency.
>
>
> Casey Schaufler
> [email protected]
--- Dave Quigley <[email protected]> wrote:
>
> ...
> > Yes, I can see that having a specific interface reduces the
> > documentation required, and simplifies it as well. Unfortunately,
> > given the way that a secctx is defined for either SELinux or
> > Smack, and the fact that the relationships between secctx values
> > are defined independently on the server and client* it does not
> > appear that the interoperability issue has been addressed, or
> > even really acknowleged with the proposed scheme. Yes, the issue
> > of label translation has been acknowleged, but it appears to me
> > that a day one solution is required for the scheme to be useful.
>
> I completely disagree here. The Linux development model isn't to code
> the entire thing throw it over a wall and then deal with the collateral
> damage. This first version assumes a heterogenous environment and from
> what we see so far that seems to be the common usecase for this
> technology. A prototype implementation is already done for label
> translations and it does need to be outlined in the RFC (Which I've
> already started doing). However it is not necessary for an initial
> release. The translation engine allows you to plug in an arbitrary
> module to support whatever LSM you are going to use so this end of the
> architecture is agnostic to the format that is going to be used on the
> wire. For now that format is just a secctx which assumes the LSM running
> on both ends is the same.
It assumes more than that. It assumes that a secctx will be
interpreted exactly the same way on both the client and the server.
On an old style MLS machine, where the label was encoded in a
data structure, this was usually a reasonable assumption, but
even then not always. Given that there is no reason to expect that
the policy on the server will match that on the client it looks
to me like you've got a day one exposure. It doesn't matter that
the LSM is the same on both ends, that's one of Trond's arguements
that I've just accepted. You have no reason to expect
interoperability even with SELinux on both ends unless you can
somehow make statements about the relative values of the secctx
on the two machines. This applies to Smack just as strongly as
it does to SELinux, so it appears the scheme is flawwed in general,
not just in the SELinux case.
I hope that's clear and specific enough. Let me know if I'm
missing something.
> Once the basics are refined and we can use it
> as a base we will keep adding more functionality (process label
> transport, better change notification, server side policy enforcement,
> translation mappings.)
>
> This is just a tiny fraction of what James outlined in the requirements
> document. So, one step at a time lest we trip over imaginary stones.
The iteritive development model has it's advantages.
Thank you.
Casey Schaufler
[email protected]
On Fri, 2008-02-29 at 14:27 -0800, Casey Schaufler wrote:
> --- Dave Quigley <[email protected]> wrote:
>
> >
> > ...
> > > Yes, I can see that having a specific interface reduces the
> > > documentation required, and simplifies it as well. Unfortunately,
> > > given the way that a secctx is defined for either SELinux or
> > > Smack, and the fact that the relationships between secctx values
> > > are defined independently on the server and client* it does not
> > > appear that the interoperability issue has been addressed, or
> > > even really acknowleged with the proposed scheme. Yes, the issue
> > > of label translation has been acknowleged, but it appears to me
> > > that a day one solution is required for the scheme to be useful.
> >
> > I completely disagree here. The Linux development model isn't to code
> > the entire thing throw it over a wall and then deal with the collateral
> > damage. This first version assumes a heterogenous environment and from
> > what we see so far that seems to be the common usecase for this
> > technology. A prototype implementation is already done for label
> > translations and it does need to be outlined in the RFC (Which I've
> > already started doing). However it is not necessary for an initial
> > release. The translation engine allows you to plug in an arbitrary
> > module to support whatever LSM you are going to use so this end of the
> > architecture is agnostic to the format that is going to be used on the
> > wire. For now that format is just a secctx which assumes the LSM running
> > on both ends is the same.
>
> It assumes more than that. It assumes that a secctx will be
> interpreted exactly the same way on both the client and the server.
> On an old style MLS machine, where the label was encoded in a
> data structure, this was usually a reasonable assumption, but
> even then not always. Given that there is no reason to expect that
> the policy on the server will match that on the client it looks
> to me like you've got a day one exposure. It doesn't matter that
> the LSM is the same on both ends, that's one of Trond's arguements
> that I've just accepted. You have no reason to expect
> interoperability even with SELinux on both ends unless you can
> somehow make statements about the relative values of the secctx
> on the two machines. This applies to Smack just as strongly as
> it does to SELinux, so it appears the scheme is flawwed in general,
> not just in the SELinux case.
Actually we can expect interoperability with SELinux on both ends. With
policy being the same on both ends it is completely valid to export a
secctx which is a user readable text representation of a label and be
able to use it on another selinux machine with the same policy. If I
have a RHEL4 and RHEL 5 box with different policies then it is the job
of the translation daemon to say I've gotten this label from this doi do
I have a mapping for it. If yes translate it into my doi. If not reject
it. This property is really no different from a user or group and I
don't see anyone suggesting we start storing those in xattrs instead of
recommended attrs.
You need to give me a specific example of why if I have policy A on both
ends on an SELinux box that a secctx isn't the same on both boxes.
>
> I hope that's clear and specific enough. Let me know if I'm
> missing something.
>
>
> > Once the basics are refined and we can use it
> > as a base we will keep adding more functionality (process label
> > transport, better change notification, server side policy enforcement,
> > translation mappings.)
> >
> > This is just a tiny fraction of what James outlined in the requirements
> > document. So, one step at a time lest we trip over imaginary stones.
>
> The iteritive development model has it's advantages.
>
> Thank you.
>
>
> Casey Schaufler
> [email protected]
--- Dave Quigley <[email protected]> wrote:
>
> ...
>
> You need to give me a specific example of why if I have policy A on both
> ends on an SELinux box that a secctx isn't the same on both boxes.
Trond can, and I'm completely confident he will, correct me if I'm
wrong, but interoperability seems to require that you can't assume
the perfect administration scenario. If you could, the name/value
pair scheme would be perfectly viable, but Trond has very clearly
explained why it is not reasonable to assume that.
But, for early going you may get away with telling people that
the configuration has to be identical. They won't listen and will
mess it up, but you will probably get away with it.
Casey Schaufler
[email protected]
On Fri, 2008-02-29 at 13:07 -0800, Casey Schaufler wrote:
> --- Trond Myklebust <[email protected]> wrote:
>
> > On Fri, 2008-02-29 at 10:52 -0800, Casey Schaufler wrote:
> > > So it sounds as if for an xattr protocol to be viable it would first
> > > require that xattr semantics be generally accepted (POSIX definition
> > > would suffice), that there be multiple implementations (Linux and Irix
> > > could suffice should Irix still be around when POSIX is done), and
> > > that there be a perceived need beyond that of the Lunitic Fringe
> > > Security Community.
> >
> > The problem isn't that of supporting the naive user xattr model: we can
> > almost do that within the existing 'named attribute' model of NFSv4. The
> > problem is that of supporting the arbitrary "security metadata" that are
> > allowed to have side-effects on the system behaviour, and that we appear
> > to have thought was a good idea to overload onto the xattr interface.
>
> Hum. Security metadata was one of the justifications for the
> original implementation of the xattr interface for XFS at SGI.
> The implementation was intended to be generic and allow for
> storage of data that impacts system behavior. No, it is not
> overloading at all, it is really supposed to be used that way.
> That's how it works on CXFS, which I know is still proprietary,
> but which could become an open peer of NFS someday.
Historical accidents change nothing to my argument. I still don't like
to be confusing user xattrs (which is a _storage_ issue) and the
security metadata (part of a _control_ protocol).
Nor do I see a compelling need to repeat any design mistakes that CXFS
might have made in this area...
> Yes, I can see that having a specific interface reduces the
> documentation required, and simplifies it as well. Unfortunately,
> given the way that a secctx is defined for either SELinux or
> Smack, and the fact that the relationships between secctx values
> are defined independently on the server and client* it does not
> appear that the interoperability issue has been addressed, or
> even really acknowleged with the proposed scheme. Yes, the issue
> of label translation has been acknowleged, but it appears to me
> that a day one solution is required for the scheme to be useful.
What would your expectation be for a SMACK-based client, if it mounts
from a server that turns out to be running with an SELinux security
model, or vice versa?
Trond
--- Trond Myklebust <[email protected]> wrote:
>
> On Fri, 2008-02-29 at 13:07 -0800, Casey Schaufler wrote:
> > --- Trond Myklebust <[email protected]> wrote:
> >
> > > On Fri, 2008-02-29 at 10:52 -0800, Casey Schaufler wrote:
> > > > So it sounds as if for an xattr protocol to be viable it would first
> > > > require that xattr semantics be generally accepted (POSIX definition
> > > > would suffice), that there be multiple implementations (Linux and Irix
> > > > could suffice should Irix still be around when POSIX is done), and
> > > > that there be a perceived need beyond that of the Lunitic Fringe
> > > > Security Community.
> > >
> > > The problem isn't that of supporting the naive user xattr model: we can
> > > almost do that within the existing 'named attribute' model of NFSv4. The
> > > problem is that of supporting the arbitrary "security metadata" that are
> > > allowed to have side-effects on the system behaviour, and that we appear
> > > to have thought was a good idea to overload onto the xattr interface.
> >
> > Hum. Security metadata was one of the justifications for the
> > original implementation of the xattr interface for XFS at SGI.
> > The implementation was intended to be generic and allow for
> > storage of data that impacts system behavior. No, it is not
> > overloading at all, it is really supposed to be used that way.
> > That's how it works on CXFS, which I know is still proprietary,
> > but which could become an open peer of NFS someday.
>
> Historical accidents change nothing to my argument. I still don't like
> to be confusing user xattrs (which is a _storage_ issue) and the
> security metadata (part of a _control_ protocol).
Twould appear that our mindsets are not in harmony.
> Nor do I see a compelling need to repeat any design mistakes that CXFS
> might have made in this area...
Oh, CXFS made mistakes, but I don't think this is one of them.
But it appears we have sufficient fundimental differences that
we'd agree on much of the list.
> > Yes, I can see that having a specific interface reduces the
> > documentation required, and simplifies it as well. Unfortunately,
> > given the way that a secctx is defined for either SELinux or
> > Smack, and the fact that the relationships between secctx values
> > are defined independently on the server and client* it does not
> > appear that the interoperability issue has been addressed, or
> > even really acknowleged with the proposed scheme. Yes, the issue
> > of label translation has been acknowleged, but it appears to me
> > that a day one solution is required for the scheme to be useful.
>
> What would your expectation be for a SMACK-based client, if it mounts
> from a server that turns out to be running with an SELinux security
> model, or vice versa?
This is a fun Friday afternoon exercise:
- SELinux server, Smack client:
Client sends "MyDogHasNoNose" to server.
Server determines that is not a value secctx as far as it knows
returns appropriate error.
Client sends "sysadm_t:so,c1,2" (some understood SELinux context) to server.
Server makes access check, goes ahead, even though the meaning of
the secctx may be unrelated.
On file creation, the file may get a secctx that the client would
not expect. Client would deny access unless the client has a rule
allowing that access.
- Smack server, SELinux client:
Client sends "sysadm_t:so,c1,2" to the server. Access checks are
made with that string. New files will get created with that label.
So long as there's a directory into which a process with that label
can write it should work with Smack semantics.
- So ...
Either could be made to function somewhat if the Smack rules
and labels got set properly. I can't claim to say that you
couldn't set up the SELinux side to accomodate the Smack labels,
but I don't think it would be easy if you can.
I think it would be a really really bad idea for anyone to
try this without both Stephen and me in the room. Dave should
be there too, so he can watch if the atmosphere catches fire.
I think that the general answer is that it wouldn't work,
but with the fate of the universe at stake and a big budget
hollywood production you could make something limp along.
Casey Schaufler
[email protected]