Commit 0837e49a authored by David Howells's avatar David Howells Committed by James Morris

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: default avatarJan Stancek <jstancek@redhat.com>
Signed-off-by: default avatarDavid Howells <dhowells@redhat.com>
Tested-by: default avatarJan Stancek <jstancek@redhat.com>
Signed-off-by: default avatarJames Morris <james.l.morris@oracle.com>
parent 6053dc98
...@@ -1151,8 +1151,21 @@ access the data: ...@@ -1151,8 +1151,21 @@ access the data:
usage. This is called key->payload.rcu_data0. The following accessors usage. This is called key->payload.rcu_data0. The following accessors
wrap the RCU calls to this element: wrap the RCU calls to this element:
rcu_assign_keypointer(struct key *key, void *data); (a) Set or change the first payload pointer:
void *rcu_dereference_key(struct key *key);
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);
=================== ===================
......
...@@ -1536,7 +1536,7 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string ...@@ -1536,7 +1536,7 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
down_read(&key->sem); down_read(&key->sem);
ukp = user_key_payload(key); ukp = user_key_payload_locked(key);
if (!ukp) { if (!ukp) {
up_read(&key->sem); up_read(&key->sem);
key_put(key); key_put(key);
......
...@@ -2455,7 +2455,7 @@ cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses) ...@@ -2455,7 +2455,7 @@ cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses)
} }
down_read(&key->sem); down_read(&key->sem);
upayload = user_key_payload(key); upayload = user_key_payload_locked(key);
if (IS_ERR_OR_NULL(upayload)) { if (IS_ERR_OR_NULL(upayload)) {
rc = upayload ? PTR_ERR(upayload) : -EINVAL; rc = upayload ? PTR_ERR(upayload) : -EINVAL;
goto out_key_put; goto out_key_put;
......
...@@ -103,7 +103,7 @@ static int validate_user_key(struct fscrypt_info *crypt_info, ...@@ -103,7 +103,7 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
goto out; goto out;
} }
down_read(&keyring_key->sem); 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)) { if (ukp->datalen != sizeof(struct fscrypt_key)) {
res = -EINVAL; res = -EINVAL;
up_read(&keyring_key->sem); up_read(&keyring_key->sem);
......
...@@ -117,7 +117,7 @@ ecryptfs_get_key_payload_data(struct key *key) ...@@ -117,7 +117,7 @@ ecryptfs_get_key_payload_data(struct key *key)
auth_tok = ecryptfs_get_encrypted_key_payload_data(key); auth_tok = ecryptfs_get_encrypted_key_payload_data(key);
if (!auth_tok) 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 else
return auth_tok; return auth_tok;
} }
......
...@@ -329,7 +329,7 @@ static void fscache_objlist_config(struct fscache_objlist_data *data) ...@@ -329,7 +329,7 @@ static void fscache_objlist_config(struct fscache_objlist_data *data)
config = 0; config = 0;
rcu_read_lock(); rcu_read_lock();
confkey = user_key_payload(key); confkey = user_key_payload_rcu(key);
buf = confkey->data; buf = confkey->data;
for (len = confkey->datalen - 1; len >= 0; len--) { for (len = confkey->datalen - 1; len >= 0; len--) {
......
...@@ -316,7 +316,7 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen, ...@@ -316,7 +316,7 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen,
if (ret < 0) if (ret < 0)
goto out_up; goto out_up;
payload = user_key_payload(rkey); payload = user_key_payload_rcu(rkey);
if (IS_ERR_OR_NULL(payload)) { if (IS_ERR_OR_NULL(payload)) {
ret = PTR_ERR(payload); ret = PTR_ERR(payload);
goto out_up; goto out_up;
......
...@@ -48,9 +48,14 @@ extern void user_describe(const struct key *user, struct seq_file *m); ...@@ -48,9 +48,14 @@ extern void user_describe(const struct key *user, struct seq_file *m);
extern long user_read(const struct key *key, extern long user_read(const struct key *key,
char __user *buffer, size_t buflen); 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 */ #endif /* CONFIG_KEYS */
......
...@@ -354,7 +354,10 @@ static inline bool key_is_instantiated(const struct key *key) ...@@ -354,7 +354,10 @@ static inline bool key_is_instantiated(const struct key *key)
!test_bit(KEY_FLAG_NEGATIVE, &key->flags); !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, \ (rcu_dereference_protected((KEY)->payload.rcu_data0, \
rwsem_is_locked(&((struct key *)(KEY))->sem))) rwsem_is_locked(&((struct key *)(KEY))->sem)))
......
...@@ -85,7 +85,7 @@ static int digsig_verify_rsa(struct key *key, ...@@ -85,7 +85,7 @@ static int digsig_verify_rsa(struct key *key,
struct pubkey_hdr *pkh; struct pubkey_hdr *pkh;
down_read(&key->sem); down_read(&key->sem);
ukp = user_key_payload(key); ukp = user_key_payload_locked(key);
if (ukp->datalen < sizeof(*pkh)) if (ukp->datalen < sizeof(*pkh))
goto err1; goto err1;
......
...@@ -70,7 +70,7 @@ int dns_query(const char *type, const char *name, size_t namelen, ...@@ -70,7 +70,7 @@ int dns_query(const char *type, const char *name, size_t namelen,
const char *options, char **_result, time64_t *_expiry) const char *options, char **_result, time64_t *_expiry)
{ {
struct key *rkey; struct key *rkey;
const struct user_key_payload *upayload; struct user_key_payload *upayload;
const struct cred *saved_cred; const struct cred *saved_cred;
size_t typelen, desclen; size_t typelen, desclen;
char *desc, *cp; char *desc, *cp;
...@@ -141,7 +141,7 @@ int dns_query(const char *type, const char *name, size_t namelen, ...@@ -141,7 +141,7 @@ int dns_query(const char *type, const char *name, size_t namelen,
if (ret) if (ret)
goto put; goto put;
upayload = user_key_payload(rkey); upayload = user_key_payload_locked(rkey);
len = upayload->datalen; len = upayload->datalen;
ret = -ENOMEM; ret = -ENOMEM;
......
...@@ -55,7 +55,7 @@ static ssize_t mpi_from_key(key_serial_t keyid, size_t maxlen, MPI *mpi) ...@@ -55,7 +55,7 @@ static ssize_t mpi_from_key(key_serial_t keyid, size_t maxlen, MPI *mpi)
if (status == 0) { if (status == 0) {
const struct user_key_payload *payload; const struct user_key_payload *payload;
payload = user_key_payload(key); payload = user_key_payload_locked(key);
if (maxlen == 0) { if (maxlen == 0) {
*mpi = NULL; *mpi = NULL;
......
...@@ -314,7 +314,7 @@ static struct key *request_user_key(const char *master_desc, const u8 **master_k ...@@ -314,7 +314,7 @@ static struct key *request_user_key(const char *master_desc, const u8 **master_k
goto error; goto error;
down_read(&ukey->sem); down_read(&ukey->sem);
upayload = user_key_payload(ukey); upayload = user_key_payload_locked(ukey);
*master_key = upayload->data; *master_key = upayload->data;
*master_keylen = upayload->datalen; *master_keylen = upayload->datalen;
error: error:
...@@ -926,7 +926,7 @@ static long encrypted_read(const struct key *key, char __user *buffer, ...@@ -926,7 +926,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
size_t asciiblob_len; size_t asciiblob_len;
int ret; int ret;
epayload = rcu_dereference_key(key); epayload = dereference_key_locked(key);
/* returns the hex encoded iv, encrypted-data, and hmac as ascii */ /* returns the hex encoded iv, encrypted-data, and hmac as ascii */
asciiblob_len = epayload->datablob_len + ivsize + 1 asciiblob_len = epayload->datablob_len + ivsize + 1
......
...@@ -1140,12 +1140,12 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) ...@@ -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, static long trusted_read(const struct key *key, char __user *buffer,
size_t buflen) size_t buflen)
{ {
struct trusted_key_payload *p; const struct trusted_key_payload *p;
char *ascii_buf; char *ascii_buf;
char *bufp; char *bufp;
int i; int i;
p = rcu_dereference_key(key); p = dereference_key_locked(key);
if (!p) if (!p)
return -EINVAL; return -EINVAL;
if (!buffer || buflen <= 0) if (!buffer || buflen <= 0)
......
...@@ -107,7 +107,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep) ...@@ -107,7 +107,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
/* attach the new data, displacing the old */ /* attach the new data, displacing the old */
key->expiry = prep->expiry; key->expiry = prep->expiry;
if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags)) 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]); rcu_assign_keypointer(key, prep->payload.data[0]);
prep->payload.data[0] = NULL; prep->payload.data[0] = NULL;
...@@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(user_update); ...@@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(user_update);
*/ */
void user_revoke(struct key *key) 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 */ /* clear the quota */
key_payload_reserve(key, 0); key_payload_reserve(key, 0);
...@@ -169,7 +169,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen) ...@@ -169,7 +169,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
const struct user_key_payload *upayload; const struct user_key_payload *upayload;
long ret; long ret;
upayload = user_key_payload(key); upayload = user_key_payload_locked(key);
ret = upayload->datalen; ret = upayload->datalen;
/* we can return the data as is */ /* we can return the data as is */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment