key payload access with just rcu_read_lock()
Subsystems (11)
cover key payload access with just rcu_read_lock()
Hi David,
this is a follow-up for "suspicious RCU usage" warning described in these 2 linux-nfs threads:
http://marc.info/?t=147558830300003&r=1&w=2
http://marc.info/?t=148776770500001&r=1&w=2
Did you have something like in mind?
Thanks,
Jan
Jan Stancek (2):
KEYS: add user_key_payload_rcu()
NFS: use user_key_payload_rcu() in RCU read-side section
Documentation/security/keys.txt | 5 +++++ fs/nfs/nfs4idmap.c | 2 +- include/keys/user-type.h | 5 +++++ include/linux/key.h | 3 +++ 4 files changed, 14 insertions(+), 1 deletion(-)
Signature · 1 line
1.8.3.1
PATCH 1/2 KEYS: add user_key_payload_rcu()
user_key_payload() is wrapper for rcu_dereference_protected(), and can't be used with just rcu_read_lock() held.
This patch adds user_key_payload_rcu() for accessing key payload in RCU read-side section, without the need to hold key semaphore.
Signed-off-by Jan Stancek <[email protected]>
--- Documentation/security/keys.txt | 5 +++++ include/keys/user-type.h | 5 +++++ include/linux/key.h | 3 +++ 3 files changed, 13 insertions(+)
Documentation/security/keys.txt +5 -0
@@ -1152,8 +1152,13 @@ access the data: wrap the RCU calls to this element: rcu_assign_keypointer(struct key *key, void *data); + + /* access payload with semaphore held */ void *rcu_dereference_key(struct key *key); + /* access payload in RCU read-side section*/ + void *rcu_read_dereference_key(struct key *key); + =================== DEFINING A KEY TYPE
include/keys/user-type.h +5 -0
@@ -53,6 +53,11 @@ static inline const struct user_key_payload *user_key_payload(const struct key *return (struct user_key_payload *)rcu_dereference_key (key); } +static inline const struct user_key_payload *user_key_payload_rcu (const struct key *key) +{ +return (struct user_key_payload *)rcu_read_dereference_key (key); +} +#endif /* CONFIG_KEYS */ #endif /* _KEYS_USER_TYPE_H */
include/linux/key.h +3 -0
@@ -358,6 +358,9 @@ static inline bool key_is_instantiated(const struct key *key) (rcu_dereference_protected ((KEY )->payload.rcu_data0, \ rwsem_is_locked(&((struct key *)(KEY))->sem))) +#define rcu_read_dereference_key (KEY ) \ + (rcu_dereference((KEY)->payload.rcu_data0)) +#define rcu_assign_keypointer (KEY ,PAYLOAD ) \ do { \ rcu_assign_pointer((KEY)->payload.rcu_data0, (PAYLOAD)); \
Signature · 1 line
1.8.3.1
PATCH 2/2 NFS: use user_key_payload_rcu() in RCU read-side section
In reply to Jan StancekUse RCU read-side accessor for key payload, as calling user_key_payload() without semaphore held leads to following warning:
===============================
[ 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: [<d000000002527abc>] 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/0xe0Signed-off-by Jan Stancek <[email protected]>
--- fs/nfs/nfs4idmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfs/nfs4idmap.c +1 -1
@@ -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;
Signature · 1 line
1.8.3.1
David Howells
maintainerQuoted 6 lines · Jan Stancek
this is a follow-up for "suspicious RCU usage" warning described in these 2 linux-nfs threads:
http://marc.info/?t=147558830300003&r=1&w=2
http://marc.info/?t=148776770500001&r=1&w=2Did you have something like in mind?
How about the attached? It's similar to what you did, but I made the split functions different from the original so that all users have to reconsider what they actually want.
David
---
commit 6b83b77697e1cec537d131f5f84e8e10c78d36e0
Author: David Howells <[email protected]> Date: Mon Feb 27 16:47:13 2017 +0000
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: [<d000000002527abc>] 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 <[email protected]>
Signed-off-by: David Howells <[email protected]>Documentation/security/keys.txt +15 -2
@@ -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); ===================
fs/fscache/object-list.c +1 -1
@@ -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--) {
fs/nfs/nfs4idmap.c +1 -1
@@ -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;
include/keys/user-type.h +7 -2
@@ -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 (struct key *key) +{ +return (struct user_key_payload *)dereference_key_locked (key); }#endif /* CONFIG_KEYS */
include/linux/key.h +4 -1
@@ -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)))
net/dns_resolver/dns_query.c +2 -2
@@ -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 ;
security/keys/dh.c +1 -1
@@ -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 ;
security/keys/encrypted-keys/encrypted.c +2 -2
@@ -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
security/keys/trusted.c +2 -2
@@ -1140,12 +1140,12 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)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 )
security/keys/user_defined.c +2 -2
@@ -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 ;
@@ -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_rcu (key); ret = upayload->datalen;/* we can return the data as is */
----- Original Message -----
Quoted 14 lines
From: "David Howells" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Sent: Monday, 27 February, 2017 11:04:21 PM
Subject: Re: [PATCH 0/2] key payload access with justrcu_read_lock()Quoted 5 lines · Jan Stancek
this is a follow-up for "suspicious RCU usage" warning described in these 2 linux-nfs threads:
http://marc.info/?t=147558830300003&r=1&w=2
http://marc.info/?t=148776770500001&r=1&w=2
Quoted 7 lines
Quoted 1 line
Did you have something like in mind?
How about the attached? It's similar to what you did, but I made the split functions different from the original so that all users have to reconsider what they actually want.
David
Looks like there are still couple users that need updating, I'm hitting following compilation error:
...
CC [M] fs/cifs/connect.ofs/cifs/connect.c: In function ‘cifs_set_cifscreds’:fs/cifs/connect.c:2442:2: error: implicit declaration of function ‘user_key_payload’ [-Werror=implicit-function-declaration]
upayload = user_key_payload(key);
^fs/cifs/connect.c:2442:11: error: assignment makes pointer from integer without a cast [-Werror]
upayload = user_key_payload(key);
^# grep "user_key_payload(" -r . --include=*.c
./drivers/md/dm-crypt.c: ukp = user_key_payload(key);
./fs/cifs/connect.c: upayload = user_key_payload(key);
./fs/crypto/keyinfo.c: ukp = user_key_payload(keyring_key);
./lib/digsig.c: ukp = user_key_payload(key);# grep "user_key_payload(" -r . --include=*.h
./fs/ecryptfs/ecryptfs_kernel.h: return (struct ecryptfs_auth_tok *)user_key_payload(key)->data;Regards,
Jan
David Howells
maintainerQuoted 2 lines · Jan Stancek
Looks like there are still couple users that need updating, I'm hitting following compilation error:
Aargh - I remembered to grep for rcu_dereference_key() but not user_key_payload().
How about the attached?
David
---
commit f0268360a253f0f86b9ec538ae3755e187229bbb
Author: David Howells <[email protected]> Date: Mon Feb 27 16:47:13 2017 +0000
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: [<d000000002527abc>] 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 <[email protected]>
Signed-off-by: David Howells <[email protected]>Documentation/security/keys.txt +15 -2
@@ -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); ===================
drivers/md/dm-crypt.c +1 -1
@@ -1536,7 +1536,7 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_stringdown_read (&key->sem); - ukp =user_key_payload (key); + ukp =user_key_payload_locked (key);if (!ukp) {up_read (&key->sem);key_put (key);
fs/cifs/connect.c +1 -1
@@ -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;
fs/crypto/keyinfo.c +1 -1
@@ -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);
fs/ecryptfs/ecryptfs_kernel.h +1 -1
@@ -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; }
fs/fscache/object-list.c +1 -1
@@ -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--) {
fs/nfs/nfs4idmap.c +1 -1
@@ -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;
include/keys/user-type.h +7 -2
@@ -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 (struct key *key) +{ +return (struct user_key_payload *)dereference_key_locked (key); }#endif /* CONFIG_KEYS */
include/linux/key.h +4 -1
@@ -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)))
lib/digsig.c +1 -1
@@ -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;
net/dns_resolver/dns_query.c +2 -2
@@ -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 ;
security/keys/dh.c +1 -1
@@ -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 ;
security/keys/encrypted-keys/encrypted.c +2 -2
@@ -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
security/keys/trusted.c +2 -2
@@ -1140,12 +1140,12 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)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 )
security/keys/user_defined.c +3 -3
@@ -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_rcu (key); ret = upayload->datalen;/* we can return the data as is */
----- Original Message -----
Quoted 9 lines
Quoted 2 lines · Jan Stancek
Looks like there are still couple users that need updating, I'm hitting following compilation error:
Aargh - I remembered to grep for
rcu_dereference_key()but notuser_key_payload().How about the attached?
v2 builds OK (on top of 4.10), "suspicious RCU usage" warning is gone and NFS connectathon is PASS-ing cleanly.
Regards,
Jan
David Howells
maintainerHere's an updated patch with fixed user_key_payload_locked() and user_read().
David
---
commit f57350ca3480c418dbc20bf73a7678a7f8e3e4ab
Author: David Howells <[email protected]> Date: Tue Feb 28 10:08:01 2017 +0000
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: [<d000000002527abc>] 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 <[email protected]>
Signed-off-by: David Howells <[email protected]>Documentation/security/keys.txt +15 -2
@@ -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); ===================
drivers/md/dm-crypt.c +1 -1
@@ -1536,7 +1536,7 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_stringdown_read (&key->sem); - ukp =user_key_payload (key); + ukp =user_key_payload_locked (key);if (!ukp) {up_read (&key->sem);key_put (key);
fs/cifs/connect.c +1 -1
@@ -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;
fs/crypto/keyinfo.c +1 -1
@@ -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);
fs/ecryptfs/ecryptfs_kernel.h +1 -1
@@ -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; }
fs/fscache/object-list.c +1 -1
@@ -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--) {
fs/nfs/nfs4idmap.c +1 -1
@@ -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;
include/keys/user-type.h +7 -2
@@ -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 */
include/linux/key.h +4 -1
@@ -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)))
lib/digsig.c +1 -1
@@ -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;
net/dns_resolver/dns_query.c +2 -2
@@ -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 ;
security/keys/dh.c +1 -1
@@ -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 ;
security/keys/encrypted-keys/encrypted.c +2 -2
@@ -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
security/keys/trusted.c +2 -2
@@ -1140,12 +1140,12 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)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 )
security/keys/user_defined.c +3 -3
@@ -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 */
----- Original Message -----
Quoted 2 lines
Here's an updated patch with fixed
user_key_payload_locked()anduser_read().
That problem didn't show up with my NFS based reproducer. I re-run it again with latest version of your patch, plus also keyutils testsuite. Both completed OK for me, dmesg looks clean.
Regards,
Jan
David Howells
maintainerQuoted 3 lines · Jan Stancek
That problem didn't show up with my NFS based reproducer. I re-run it again with latest version of your patch, plus also keyutils testsuite. Both completed OK for me, dmesg looks clean.
Can I put you down as a Tested-by?
David
----- Original Message -----
Quoted 7 lines
From: "David Howells" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Sent: Wednesday, 1 March, 2017 10:40:13 AM
Subject: Re: [PATCH 0/2] key payload access with justrcu_read_lock()
Quoted 7 lines
Quoted 3 lines · Jan Stancek
That problem didn't show up with my NFS based reproducer. I re-run it again with latest version of your patch, plus also keyutils testsuite. Both completed OK for me, dmesg looks clean.
Can I put you down as a Tested-by?
Yes. Thanks for having a look at this.
Regards,
Jan