From 0837e49ab3fa8d903a499984575d71efee8097ce Mon Sep 17 00:00:00 2001 From: David Howells Date: Wed, 1 Mar 2017 15:11:23 +0000 Subject: [PATCH 1/2] KEYS: Differentiate uses of rcu_dereference_key() and user_key_payload() rcu_dereference_key() and user_key_payload() are currently being used in two different, incompatible ways: (1) As a wrapper to rcu_dereference() - when only the RCU read lock used to protect the key. (2) As a wrapper to rcu_dereference_protected() - when the key semaphor is used to protect the key and the may be being modified. Fix this by splitting both of the key wrappers to produce: (1) RCU accessors for keys when caller has the key semaphore locked: dereference_key_locked() user_key_payload_locked() (2) RCU accessors for keys when caller holds the RCU read lock: dereference_key_rcu() user_key_payload_rcu() This should fix following warning in the NFS idmapper =============================== [ INFO: suspicious RCU usage. ] 4.10.0 #1 Tainted: G W ------------------------------- ./include/keys/user-type.h:53 suspicious rcu_dereference_protected() usage! other info that might help us debug this: rcu_scheduler_active = 2, debug_locks = 0 1 lock held by mount.nfs/5987: #0: (rcu_read_lock){......}, at: [] nfs_idmap_get_key+0x15c/0x420 [nfsv4] stack backtrace: CPU: 1 PID: 5987 Comm: mount.nfs Tainted: G W 4.10.0 #1 Call Trace: dump_stack+0xe8/0x154 (unreliable) lockdep_rcu_suspicious+0x140/0x190 nfs_idmap_get_key+0x380/0x420 [nfsv4] nfs_map_name_to_uid+0x2a0/0x3b0 [nfsv4] decode_getfattr_attrs+0xfac/0x16b0 [nfsv4] decode_getfattr_generic.constprop.106+0xbc/0x150 [nfsv4] nfs4_xdr_dec_lookup_root+0xac/0xb0 [nfsv4] rpcauth_unwrap_resp+0xe8/0x140 [sunrpc] call_decode+0x29c/0x910 [sunrpc] __rpc_execute+0x140/0x8f0 [sunrpc] rpc_run_task+0x170/0x200 [sunrpc] nfs4_call_sync_sequence+0x68/0xa0 [nfsv4] _nfs4_lookup_root.isra.44+0xd0/0xf0 [nfsv4] nfs4_lookup_root+0xe0/0x350 [nfsv4] nfs4_lookup_root_sec+0x70/0xa0 [nfsv4] nfs4_find_root_sec+0xc4/0x100 [nfsv4] nfs4_proc_get_rootfh+0x5c/0xf0 [nfsv4] nfs4_get_rootfh+0x6c/0x190 [nfsv4] nfs4_server_common_setup+0xc4/0x260 [nfsv4] nfs4_create_server+0x278/0x3c0 [nfsv4] nfs4_remote_mount+0x50/0xb0 [nfsv4] mount_fs+0x74/0x210 vfs_kern_mount+0x78/0x220 nfs_do_root_mount+0xb0/0x140 [nfsv4] nfs4_try_mount+0x60/0x100 [nfsv4] nfs_fs_mount+0x5ec/0xda0 [nfs] mount_fs+0x74/0x210 vfs_kern_mount+0x78/0x220 do_mount+0x254/0xf70 SyS_mount+0x94/0x100 system_call+0x38/0xe0 Reported-by: Jan Stancek Signed-off-by: David Howells Tested-by: Jan Stancek Signed-off-by: James Morris --- Documentation/security/keys.txt | 17 +++++++++++++++-- drivers/md/dm-crypt.c | 2 +- fs/cifs/connect.c | 2 +- fs/crypto/keyinfo.c | 2 +- fs/ecryptfs/ecryptfs_kernel.h | 2 +- fs/fscache/object-list.c | 2 +- fs/nfs/nfs4idmap.c | 2 +- include/keys/user-type.h | 9 +++++++-- include/linux/key.h | 5 ++++- lib/digsig.c | 2 +- net/dns_resolver/dns_query.c | 4 ++-- security/keys/dh.c | 2 +- security/keys/encrypted-keys/encrypted.c | 4 ++-- security/keys/trusted.c | 4 ++-- security/keys/user_defined.c | 6 +++--- 15 files changed, 43 insertions(+), 22 deletions(-) diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt index 3849814bfe6d..0e03baf271bd 100644 --- a/Documentation/security/keys.txt +++ b/Documentation/security/keys.txt @@ -1151,8 +1151,21 @@ access the data: usage. This is called key->payload.rcu_data0. The following accessors wrap the RCU calls to this element: - rcu_assign_keypointer(struct key *key, void *data); - void *rcu_dereference_key(struct key *key); + (a) Set or change the first payload pointer: + + rcu_assign_keypointer(struct key *key, void *data); + + (b) Read the first payload pointer with the key semaphore held: + + [const] void *dereference_key_locked([const] struct key *key); + + Note that the return value will inherit its constness from the key + parameter. Static analysis will give an error if it things the lock + isn't held. + + (c) Read the first payload pointer with the RCU read lock held: + + const void *dereference_key_rcu(const struct key *key); =================== diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 1cb2ca9dfae3..389a3637ffcc 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -1536,7 +1536,7 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string down_read(&key->sem); - ukp = user_key_payload(key); + ukp = user_key_payload_locked(key); if (!ukp) { up_read(&key->sem); key_put(key); diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 777ad9f4fc3c..8a3ecef30d3c 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2455,7 +2455,7 @@ cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses) } down_read(&key->sem); - upayload = user_key_payload(key); + upayload = user_key_payload_locked(key); if (IS_ERR_OR_NULL(upayload)) { rc = upayload ? PTR_ERR(upayload) : -EINVAL; goto out_key_put; diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c index 02eb6b9e4438..d5d896fa5a71 100644 --- a/fs/crypto/keyinfo.c +++ b/fs/crypto/keyinfo.c @@ -103,7 +103,7 @@ static int validate_user_key(struct fscrypt_info *crypt_info, goto out; } down_read(&keyring_key->sem); - ukp = user_key_payload(keyring_key); + ukp = user_key_payload_locked(keyring_key); if (ukp->datalen != sizeof(struct fscrypt_key)) { res = -EINVAL; up_read(&keyring_key->sem); diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h index 599a29237cfe..95c1c8d34539 100644 --- a/fs/ecryptfs/ecryptfs_kernel.h +++ b/fs/ecryptfs/ecryptfs_kernel.h @@ -117,7 +117,7 @@ ecryptfs_get_key_payload_data(struct key *key) auth_tok = ecryptfs_get_encrypted_key_payload_data(key); if (!auth_tok) - return (struct ecryptfs_auth_tok *)user_key_payload(key)->data; + return (struct ecryptfs_auth_tok *)user_key_payload_locked(key)->data; else return auth_tok; } diff --git a/fs/fscache/object-list.c b/fs/fscache/object-list.c index 5d5ddaa84b21..67f940892ef8 100644 --- a/fs/fscache/object-list.c +++ b/fs/fscache/object-list.c @@ -329,7 +329,7 @@ static void fscache_objlist_config(struct fscache_objlist_data *data) config = 0; rcu_read_lock(); - confkey = user_key_payload(key); + confkey = user_key_payload_rcu(key); buf = confkey->data; for (len = confkey->datalen - 1; len >= 0; len--) { diff --git a/fs/nfs/nfs4idmap.c b/fs/nfs/nfs4idmap.c index c444285bb1b1..835c163f61af 100644 --- a/fs/nfs/nfs4idmap.c +++ b/fs/nfs/nfs4idmap.c @@ -316,7 +316,7 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen, if (ret < 0) goto out_up; - payload = user_key_payload(rkey); + payload = user_key_payload_rcu(rkey); if (IS_ERR_OR_NULL(payload)) { ret = PTR_ERR(payload); goto out_up; diff --git a/include/keys/user-type.h b/include/keys/user-type.h index c56fef40f53e..e098cbe27db5 100644 --- a/include/keys/user-type.h +++ b/include/keys/user-type.h @@ -48,9 +48,14 @@ extern void user_describe(const struct key *user, struct seq_file *m); extern long user_read(const struct key *key, char __user *buffer, size_t buflen); -static inline const struct user_key_payload *user_key_payload(const struct key *key) +static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key) { - return (struct user_key_payload *)rcu_dereference_key(key); + return (struct user_key_payload *)dereference_key_rcu(key); +} + +static inline struct user_key_payload *user_key_payload_locked(const struct key *key) +{ + return (struct user_key_payload *)dereference_key_locked((struct key *)key); } #endif /* CONFIG_KEYS */ diff --git a/include/linux/key.h b/include/linux/key.h index 722914798f37..e45212f2777e 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -354,7 +354,10 @@ static inline bool key_is_instantiated(const struct key *key) !test_bit(KEY_FLAG_NEGATIVE, &key->flags); } -#define rcu_dereference_key(KEY) \ +#define dereference_key_rcu(KEY) \ + (rcu_dereference((KEY)->payload.rcu_data0)) + +#define dereference_key_locked(KEY) \ (rcu_dereference_protected((KEY)->payload.rcu_data0, \ rwsem_is_locked(&((struct key *)(KEY))->sem))) diff --git a/lib/digsig.c b/lib/digsig.c index 55b8b2f41a9e..03d7c63837ae 100644 --- a/lib/digsig.c +++ b/lib/digsig.c @@ -85,7 +85,7 @@ static int digsig_verify_rsa(struct key *key, struct pubkey_hdr *pkh; down_read(&key->sem); - ukp = user_key_payload(key); + ukp = user_key_payload_locked(key); if (ukp->datalen < sizeof(*pkh)) goto err1; diff --git a/net/dns_resolver/dns_query.c b/net/dns_resolver/dns_query.c index ecc28cff08ab..d502c94b1a82 100644 --- a/net/dns_resolver/dns_query.c +++ b/net/dns_resolver/dns_query.c @@ -70,7 +70,7 @@ int dns_query(const char *type, const char *name, size_t namelen, const char *options, char **_result, time64_t *_expiry) { struct key *rkey; - const struct user_key_payload *upayload; + struct user_key_payload *upayload; const struct cred *saved_cred; size_t typelen, desclen; char *desc, *cp; @@ -141,7 +141,7 @@ int dns_query(const char *type, const char *name, size_t namelen, if (ret) goto put; - upayload = user_key_payload(rkey); + upayload = user_key_payload_locked(rkey); len = upayload->datalen; ret = -ENOMEM; diff --git a/security/keys/dh.c b/security/keys/dh.c index 531ed2ec132f..893af4c45038 100644 --- a/security/keys/dh.c +++ b/security/keys/dh.c @@ -55,7 +55,7 @@ static ssize_t mpi_from_key(key_serial_t keyid, size_t maxlen, MPI *mpi) if (status == 0) { const struct user_key_payload *payload; - payload = user_key_payload(key); + payload = user_key_payload_locked(key); if (maxlen == 0) { *mpi = NULL; diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 4fb315cddf5b..0010955d7876 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -314,7 +314,7 @@ static struct key *request_user_key(const char *master_desc, const u8 **master_k goto error; down_read(&ukey->sem); - upayload = user_key_payload(ukey); + upayload = user_key_payload_locked(ukey); *master_key = upayload->data; *master_keylen = upayload->datalen; error: @@ -926,7 +926,7 @@ static long encrypted_read(const struct key *key, char __user *buffer, size_t asciiblob_len; int ret; - epayload = rcu_dereference_key(key); + epayload = dereference_key_locked(key); /* returns the hex encoded iv, encrypted-data, and hmac as ascii */ asciiblob_len = epayload->datablob_len + ivsize + 1 diff --git a/security/keys/trusted.c b/security/keys/trusted.c index 90d61751ff12..2ae31c5a87de 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -1140,12 +1140,12 @@ out: static long trusted_read(const struct key *key, char __user *buffer, size_t buflen) { - struct trusted_key_payload *p; + const struct trusted_key_payload *p; char *ascii_buf; char *bufp; int i; - p = rcu_dereference_key(key); + p = dereference_key_locked(key); if (!p) return -EINVAL; if (!buffer || buflen <= 0) diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c index e187c8909d9d..26605134f17a 100644 --- a/security/keys/user_defined.c +++ b/security/keys/user_defined.c @@ -107,7 +107,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep) /* attach the new data, displacing the old */ key->expiry = prep->expiry; if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags)) - zap = rcu_dereference_key(key); + zap = dereference_key_locked(key); rcu_assign_keypointer(key, prep->payload.data[0]); prep->payload.data[0] = NULL; @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(user_update); */ void user_revoke(struct key *key) { - struct user_key_payload *upayload = key->payload.data[0]; + struct user_key_payload *upayload = user_key_payload_locked(key); /* clear the quota */ key_payload_reserve(key, 0); @@ -169,7 +169,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen) const struct user_key_payload *upayload; long ret; - upayload = user_key_payload(key); + upayload = user_key_payload_locked(key); ret = upayload->datalen; /* we can return the data as is */ From 2651225b5ebcdde60f684c4db8ec7e9e3800a74f Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Tue, 28 Feb 2017 10:35:56 -0500 Subject: [PATCH 2/2] selinux: wrap cgroup seclabel support with its own policy capability commit 1ea0ce40690dff38935538e8dab7b12683ded0d3 ("selinux: allow changing labels for cgroupfs") broke the Android init program, which looks up security contexts whenever creating directories and attempts to assign them via setfscreatecon(). When creating subdirectories in cgroup mounts, this would previously be ignored since cgroup did not support userspace setting of security contexts. However, after the commit, SELinux would attempt to honor the requested context on cgroup directories and fail due to permission denial. Avoid breaking existing userspace/policy by wrapping this change with a conditional on a new cgroup_seclabel policy capability. This preserves existing behavior until/unless a new policy explicitly enables this capability. Reported-by: John Stultz Signed-off-by: Stephen Smalley Signed-off-by: Paul Moore Signed-off-by: James Morris --- security/selinux/hooks.c | 7 ++++--- security/selinux/include/security.h | 2 ++ security/selinux/selinuxfs.c | 3 ++- security/selinux/ss/services.c | 4 ++++ 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 9a8f12f8d5b7..0a4b4b040e0a 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -480,12 +480,13 @@ static int selinux_is_sblabel_mnt(struct super_block *sb) sbsec->behavior == SECURITY_FS_USE_NATIVE || /* Special handling. Genfs but also in-core setxattr handler */ !strcmp(sb->s_type->name, "sysfs") || - !strcmp(sb->s_type->name, "cgroup") || - !strcmp(sb->s_type->name, "cgroup2") || !strcmp(sb->s_type->name, "pstore") || !strcmp(sb->s_type->name, "debugfs") || !strcmp(sb->s_type->name, "tracefs") || - !strcmp(sb->s_type->name, "rootfs"); + !strcmp(sb->s_type->name, "rootfs") || + (selinux_policycap_cgroupseclabel && + (!strcmp(sb->s_type->name, "cgroup") || + !strcmp(sb->s_type->name, "cgroup2"))); } static int sb_finish_set_opts(struct super_block *sb) diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index beaa14b8b6cf..f979c35e037e 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -71,6 +71,7 @@ enum { POLICYDB_CAPABILITY_OPENPERM, POLICYDB_CAPABILITY_EXTSOCKCLASS, POLICYDB_CAPABILITY_ALWAYSNETWORK, + POLICYDB_CAPABILITY_CGROUPSECLABEL, __POLICYDB_CAPABILITY_MAX }; #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) @@ -79,6 +80,7 @@ extern int selinux_policycap_netpeer; extern int selinux_policycap_openperm; extern int selinux_policycap_extsockclass; extern int selinux_policycap_alwaysnetwork; +extern int selinux_policycap_cgroupseclabel; /* * type_datum properties diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index c9e8a9898ce4..cb3fd98fb05a 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -46,7 +46,8 @@ static char *policycap_names[] = { "network_peer_controls", "open_perms", "extended_socket_class", - "always_check_network" + "always_check_network", + "cgroup_seclabel" }; unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index a70fcee9824b..b4aa491a0a23 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -74,6 +74,7 @@ int selinux_policycap_netpeer; int selinux_policycap_openperm; int selinux_policycap_extsockclass; int selinux_policycap_alwaysnetwork; +int selinux_policycap_cgroupseclabel; static DEFINE_RWLOCK(policy_rwlock); @@ -1993,6 +1994,9 @@ static void security_load_policycaps(void) POLICYDB_CAPABILITY_EXTSOCKCLASS); selinux_policycap_alwaysnetwork = ebitmap_get_bit(&policydb.policycaps, POLICYDB_CAPABILITY_ALWAYSNETWORK); + selinux_policycap_cgroupseclabel = + ebitmap_get_bit(&policydb.policycaps, + POLICYDB_CAPABILITY_CGROUPSECLABEL); } static int security_preserve_bools(struct policydb *p);