Commit 4c205c84 authored by Linus Torvalds's avatar Linus Torvalds

Merge tag 'keys-fixes-20200329' of git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs

Pull keyrings fixes from David Howells:
 "Here's a couple of patches that fix a circular dependency between
  holding key->sem and mm->mmap_sem when reading data from a key.

  One potential issue is that a filesystem looking to use a key inside,
  say, ->readpages() could deadlock if the key being read is the key
  that's required and the buffer the key is being read into is on a page
  that needs to be fetched.

  The case actually detected is a bit more involved - with a filesystem
  calling request_key() and locking the target keyring for write - which
  could be being read"

* tag 'keys-fixes-20200329' of git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs:
  KEYS: Avoid false positive ENOMEM error on key read
  KEYS: Don't write out to userspace while holding key semaphore
parents ea9448b2 4f088249
......@@ -17,6 +17,6 @@ extern void big_key_free_preparse(struct key_preparsed_payload *prep);
extern void big_key_revoke(struct key *key);
extern void big_key_destroy(struct key *key);
extern void big_key_describe(const struct key *big_key, struct seq_file *m);
extern long big_key_read(const struct key *key, char __user *buffer, size_t buflen);
extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
#endif /* _KEYS_BIG_KEY_TYPE_H */
......@@ -41,8 +41,7 @@ extern int user_update(struct key *key, struct key_preparsed_payload *prep);
extern void user_revoke(struct key *key);
extern void user_destroy(struct key *key);
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);
extern long user_read(const struct key *key, char *buffer, size_t buflen);
static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key)
{
......
......@@ -127,7 +127,7 @@ struct key_type {
* much is copied into the buffer
* - shouldn't do the copy if the buffer is NULL
*/
long (*read)(const struct key *key, char __user *buffer, size_t buflen);
long (*read)(const struct key *key, char *buffer, size_t buflen);
/* handle request_key() for this type instead of invoking
* /sbin/request-key (optional)
......
......@@ -302,7 +302,7 @@ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
* - the key's semaphore is read-locked
*/
static long dns_resolver_read(const struct key *key,
char __user *buffer, size_t buflen)
char *buffer, size_t buflen)
{
int err = PTR_ERR(key->payload.data[dns_key_error]);
......
......@@ -31,7 +31,7 @@ static void rxrpc_free_preparse_s(struct key_preparsed_payload *);
static void rxrpc_destroy(struct key *);
static void rxrpc_destroy_s(struct key *);
static void rxrpc_describe(const struct key *, struct seq_file *);
static long rxrpc_read(const struct key *, char __user *, size_t);
static long rxrpc_read(const struct key *, char *, size_t);
/*
* rxrpc defined keys take an arbitrary string as the description and an
......@@ -1042,12 +1042,12 @@ EXPORT_SYMBOL(rxrpc_get_null_key);
* - this returns the result in XDR form
*/
static long rxrpc_read(const struct key *key,
char __user *buffer, size_t buflen)
char *buffer, size_t buflen)
{
const struct rxrpc_key_token *token;
const struct krb5_principal *princ;
size_t size;
__be32 __user *xdr, *oldxdr;
__be32 *xdr, *oldxdr;
u32 cnlen, toksize, ntoks, tok, zero;
u16 toksizes[AFSTOKEN_MAX];
int loop;
......@@ -1124,30 +1124,25 @@ static long rxrpc_read(const struct key *key,
if (!buffer || buflen < size)
return size;
xdr = (__be32 __user *) buffer;
xdr = (__be32 *)buffer;
zero = 0;
#define ENCODE(x) \
do { \
__be32 y = htonl(x); \
if (put_user(y, xdr++) < 0) \
goto fault; \
*xdr++ = htonl(x); \
} while(0)
#define ENCODE_DATA(l, s) \
do { \
u32 _l = (l); \
ENCODE(l); \
if (copy_to_user(xdr, (s), _l) != 0) \
goto fault; \
if (_l & 3 && \
copy_to_user((u8 __user *)xdr + _l, &zero, 4 - (_l & 3)) != 0) \
goto fault; \
memcpy(xdr, (s), _l); \
if (_l & 3) \
memcpy((u8 *)xdr + _l, &zero, 4 - (_l & 3)); \
xdr += (_l + 3) >> 2; \
} while(0)
#define ENCODE64(x) \
do { \
__be64 y = cpu_to_be64(x); \
if (copy_to_user(xdr, &y, 8) != 0) \
goto fault; \
memcpy(xdr, &y, 8); \
xdr += 8 >> 2; \
} while(0)
#define ENCODE_STR(s) \
......@@ -1238,8 +1233,4 @@ static long rxrpc_read(const struct key *key,
ASSERTCMP((char __user *) xdr - buffer, ==, size);
_leave(" = %zu", size);
return size;
fault:
_leave(" = -EFAULT");
return -EFAULT;
}
......@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
* read the key data
* - the key's semaphore is read-locked
*/
long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
long big_key_read(const struct key *key, char *buffer, size_t buflen)
{
size_t datalen = (size_t)key->payload.data[big_key_len];
long ret;
......@@ -391,9 +391,8 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
ret = datalen;
/* copy decrypted data to user */
if (copy_to_user(buffer, buf->virt, datalen) != 0)
ret = -EFAULT;
/* copy out decrypted data */
memcpy(buffer, buf->virt, datalen);
err_fput:
fput(file);
......@@ -401,9 +400,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
big_key_free_buffer(buf);
} else {
ret = datalen;
if (copy_to_user(buffer, key->payload.data[big_key_data],
datalen) != 0)
ret = -EFAULT;
memcpy(buffer, key->payload.data[big_key_data], datalen);
}
return ret;
......
......@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
}
/*
* encrypted_read - format and copy the encrypted data to userspace
* encrypted_read - format and copy out the encrypted data
*
* The resulting datablob format is:
* <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
*
* On success, return to userspace the encrypted key datablob size.
*/
static long encrypted_read(const struct key *key, char __user *buffer,
static long encrypted_read(const struct key *key, char *buffer,
size_t buflen)
{
struct encrypted_key_payload *epayload;
......@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
key_put(mkey);
memzero_explicit(derived_key, sizeof(derived_key));
if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
ret = -EFAULT;
memcpy(buffer, ascii_buf, asciiblob_len);
kzfree(ascii_buf);
return asciiblob_len;
......
......@@ -16,6 +16,8 @@
#include <linux/keyctl.h>
#include <linux/refcount.h>
#include <linux/compat.h>
#include <linux/mm.h>
#include <linux/vmalloc.h>
struct iovec;
......@@ -349,4 +351,14 @@ static inline void key_check(const struct key *key)
#endif
/*
* Helper function to clear and free a kvmalloc'ed memory object.
*/
static inline void __kvzfree(const void *addr, size_t len)
{
if (addr) {
memset((void *)addr, 0, len);
kvfree(addr);
}
}
#endif /* _INTERNAL_H */
......@@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
payload = NULL;
if (plen) {
ret = -ENOMEM;
payload = kmalloc(plen, GFP_KERNEL);
payload = kvmalloc(plen, GFP_KERNEL);
if (!payload)
goto error;
......@@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
key_ref_put(key_ref);
error2:
kzfree(payload);
__kvzfree(payload, plen);
error:
return ret;
}
......@@ -797,6 +797,21 @@ long keyctl_keyring_search(key_serial_t ringid,
return ret;
}
/*
* Call the read method
*/
static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
{
long ret;
down_read(&key->sem);
ret = key_validate(key);
if (ret == 0)
ret = key->type->read(key, buffer, buflen);
up_read(&key->sem);
return ret;
}
/*
* Read a key's payload.
*
......@@ -812,26 +827,28 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
struct key *key;
key_ref_t key_ref;
long ret;
char *key_data = NULL;
size_t key_data_len;
/* find the key first */
key_ref = lookup_user_key(keyid, 0, 0);
if (IS_ERR(key_ref)) {
ret = -ENOKEY;
goto error;
goto out;
}
key = key_ref_to_ptr(key_ref);
ret = key_read_state(key);
if (ret < 0)
goto error2; /* Negatively instantiated */
goto key_put_out; /* Negatively instantiated */
/* see if we can read it directly */
ret = key_permission(key_ref, KEY_NEED_READ);
if (ret == 0)
goto can_read_key;
if (ret != -EACCES)
goto error2;
goto key_put_out;
/* we can't; see if it's searchable from this process's keyrings
* - we automatically take account of the fact that it may be
......@@ -839,26 +856,78 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
*/
if (!is_key_possessed(key_ref)) {
ret = -EACCES;
goto error2;
goto key_put_out;
}
/* the key is probably readable - now try to read it */
can_read_key:
if (!key->type->read) {
ret = -EOPNOTSUPP;
if (key->type->read) {
/* Read the data with the semaphore held (since we might sleep)
goto key_put_out;
}
if (!buffer || !buflen) {
/* Get the key length from the read method */
ret = __keyctl_read_key(key, NULL, 0);
goto key_put_out;
}
/*
* Read the data with the semaphore held (since we might sleep)
* to protect against the key being updated or revoked.
*
* Allocating a temporary buffer to hold the keys before
* transferring them to user buffer to avoid potential
* deadlock involving page fault and mmap_sem.
*
* key_data_len = (buflen <= PAGE_SIZE)
* ? buflen : actual length of key data
*
* This prevents allocating arbitrary large buffer which can
* be much larger than the actual key length. In the latter case,
* at least 2 passes of this loop is required.
*/
down_read(&key->sem);
ret = key_validate(key);
if (ret == 0)
ret = key->type->read(key, buffer, buflen);
up_read(&key->sem);
key_data_len = (buflen <= PAGE_SIZE) ? buflen : 0;
for (;;) {
if (key_data_len) {
key_data = kvmalloc(key_data_len, GFP_KERNEL);
if (!key_data) {
ret = -ENOMEM;
goto key_put_out;
}
}
error2:
ret = __keyctl_read_key(key, key_data, key_data_len);
/*
* Read methods will just return the required length without
* any copying if the provided length isn't large enough.
*/
if (ret <= 0 || ret > buflen)
break;
/*
* The key may change (unlikely) in between 2 consecutive
* __keyctl_read_key() calls. In this case, we reallocate
* a larger buffer and redo the key read when
* key_data_len < ret <= buflen.
*/
if (ret > key_data_len) {
if (unlikely(key_data))
__kvzfree(key_data, key_data_len);
key_data_len = ret;
continue; /* Allocate buffer */
}
if (copy_to_user(buffer, key_data, ret))
ret = -EFAULT;
break;
}
__kvzfree(key_data, key_data_len);
key_put_out:
key_put(key);
error:
out:
return ret;
}
......
......@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
{
struct keyring_read_iterator_context *ctx = data;
const struct key *key = keyring_ptr_to_key(object);
int ret;
kenter("{%s,%d},,{%zu/%zu}",
key->type->name, key->serial, ctx->count, ctx->buflen);
......@@ -467,10 +466,7 @@ static int keyring_read_iterator(const void *object, void *data)
if (ctx->count >= ctx->buflen)
return 1;
ret = put_user(key->serial, ctx->buffer);
if (ret < 0)
return ret;
ctx->buffer++;
*ctx->buffer++ = key->serial;
ctx->count += sizeof(key->serial);
return 0;
}
......
......@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
static void request_key_auth_describe(const struct key *, struct seq_file *);
static void request_key_auth_revoke(struct key *);
static void request_key_auth_destroy(struct key *);
static long request_key_auth_read(const struct key *, char __user *, size_t);
static long request_key_auth_read(const struct key *, char *, size_t);
/*
* The request-key authorisation key type definition.
......@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
* - the key's semaphore is read-locked
*/
static long request_key_auth_read(const struct key *key,
char __user *buffer, size_t buflen)
char *buffer, size_t buflen)
{
struct request_key_auth *rka = dereference_key_locked(key);
size_t datalen;
......@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
if (buflen > datalen)
buflen = datalen;
if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
ret = -EFAULT;
memcpy(buffer, rka->callout_info, buflen);
}
return ret;
......
......@@ -1130,11 +1130,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
* trusted_read - copy the sealed blob data to userspace in hex.
* On success, return to userspace the trusted key datablob size.
*/
static long trusted_read(const struct key *key, char __user *buffer,
static long trusted_read(const struct key *key, char *buffer,
size_t buflen)
{
const struct trusted_key_payload *p;
char *ascii_buf;
char *bufp;
int i;
......@@ -1143,18 +1142,9 @@ static long trusted_read(const struct key *key, char __user *buffer,
return -EINVAL;
if (buffer && buflen >= 2 * p->blob_len) {
ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
if (!ascii_buf)
return -ENOMEM;
bufp = ascii_buf;
bufp = buffer;
for (i = 0; i < p->blob_len; i++)
bufp = hex_byte_pack(bufp, p->blob[i]);
if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
kzfree(ascii_buf);
return -EFAULT;
}
kzfree(ascii_buf);
}
return 2 * p->blob_len;
}
......
......@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(user_describe);
* read the key data
* - the key's semaphore is read-locked
*/
long user_read(const struct key *key, char __user *buffer, size_t buflen)
long user_read(const struct key *key, char *buffer, size_t buflen)
{
const struct user_key_payload *upayload;
long ret;
......@@ -181,8 +181,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
if (buflen > upayload->datalen)
buflen = upayload->datalen;
if (copy_to_user(buffer, upayload->data, buflen) != 0)
ret = -EFAULT;
memcpy(buffer, upayload->data, buflen);
}
return ret;
......
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