* [PATCH v4 1/4] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-17 19:41 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200317194140.6031-1-longman@redhat.com>
A lockdep circular locking dependency report was seen when running a
keyutils test:
[12537.027242] ======================================================
[12537.059309] WARNING: possible circular locking dependency detected
[12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE --------- - -
[12537.125253] ------------------------------------------------------
[12537.153189] keyctl/25598 is trying to acquire lock:
[12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
[12537.208365]
[12537.208365] but task is already holding lock:
[12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12537.270476]
[12537.270476] which lock already depends on the new lock.
[12537.270476]
[12537.307209]
[12537.307209] the existing dependency chain (in reverse order) is:
[12537.340754]
[12537.340754] -> #3 (&type->lock_class){++++}:
[12537.367434] down_write+0x4d/0x110
[12537.385202] __key_link_begin+0x87/0x280
[12537.405232] request_key_and_link+0x483/0xf70
[12537.427221] request_key+0x3c/0x80
[12537.444839] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.468445] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.496731] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.519418] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.546263] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.573551] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.601045] kthread+0x30c/0x3d0
[12537.617906] ret_from_fork+0x3a/0x50
[12537.636225]
[12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
[12537.664525] __mutex_lock+0x105/0x11f0
[12537.683734] request_key_and_link+0x35a/0xf70
[12537.705640] request_key+0x3c/0x80
[12537.723304] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.746773] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.775607] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.798322] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.823369] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.847262] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.873477] kthread+0x30c/0x3d0
[12537.890281] ret_from_fork+0x3a/0x50
[12537.908649]
[12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
[12537.935225] __mutex_lock+0x105/0x11f0
[12537.954450] cifs_call_async+0x102/0x7f0 [cifs]
[12537.977250] smb2_async_readv+0x6c3/0xc90 [cifs]
[12538.000659] cifs_readpages+0x120a/0x1e50 [cifs]
[12538.023920] read_pages+0xf5/0x560
[12538.041583] __do_page_cache_readahead+0x41d/0x4b0
[12538.067047] ondemand_readahead+0x44c/0xc10
[12538.092069] filemap_fault+0xec1/0x1830
[12538.111637] __do_fault+0x82/0x260
[12538.129216] do_fault+0x419/0xfb0
[12538.146390] __handle_mm_fault+0x862/0xdf0
[12538.167408] handle_mm_fault+0x154/0x550
[12538.187401] __do_page_fault+0x42f/0xa60
[12538.207395] do_page_fault+0x38/0x5e0
[12538.225777] page_fault+0x1e/0x30
[12538.243010]
[12538.243010] -> #0 (&mm->mmap_sem){++++}:
[12538.267875] lock_acquire+0x14c/0x420
[12538.286848] __might_fault+0x119/0x1b0
[12538.306006] keyring_read_iterator+0x7e/0x170
[12538.327936] assoc_array_subtree_iterate+0x97/0x280
[12538.352154] keyring_read+0xe9/0x110
[12538.370558] keyctl_read_key+0x1b9/0x220
[12538.391470] do_syscall_64+0xa5/0x4b0
[12538.410511] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
[12538.435535]
[12538.435535] other info that might help us debug this:
[12538.435535]
[12538.472829] Chain exists of:
[12538.472829] &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
[12538.472829]
[12538.524820] Possible unsafe locking scenario:
[12538.524820]
[12538.551431] CPU0 CPU1
[12538.572654] ---- ----
[12538.595865] lock(&type->lock_class);
[12538.613737] lock(root_key_user.cons_lock);
[12538.644234] lock(&type->lock_class);
[12538.672410] lock(&mm->mmap_sem);
[12538.687758]
[12538.687758] *** DEADLOCK ***
[12538.687758]
[12538.714455] 1 lock held by keyctl/25598:
[12538.732097] #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12538.770573]
[12538.770573] stack backtrace:
[12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
[12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
[12538.881963] Call Trace:
[12538.892897] dump_stack+0x9a/0xf0
[12538.907908] print_circular_bug.isra.25.cold.50+0x1bc/0x279
[12538.932891] ? save_trace+0xd6/0x250
[12538.948979] check_prev_add.constprop.32+0xc36/0x14f0
[12538.971643] ? keyring_compare_object+0x104/0x190
[12538.992738] ? check_usage+0x550/0x550
[12539.009845] ? sched_clock+0x5/0x10
[12539.025484] ? sched_clock_cpu+0x18/0x1e0
[12539.043555] __lock_acquire+0x1f12/0x38d0
[12539.061551] ? trace_hardirqs_on+0x10/0x10
[12539.080554] lock_acquire+0x14c/0x420
[12539.100330] ? __might_fault+0xc4/0x1b0
[12539.119079] __might_fault+0x119/0x1b0
[12539.135869] ? __might_fault+0xc4/0x1b0
[12539.153234] keyring_read_iterator+0x7e/0x170
[12539.172787] ? keyring_read+0x110/0x110
[12539.190059] assoc_array_subtree_iterate+0x97/0x280
[12539.211526] keyring_read+0xe9/0x110
[12539.227561] ? keyring_gc_check_iterator+0xc0/0xc0
[12539.249076] keyctl_read_key+0x1b9/0x220
[12539.266660] do_syscall_64+0xa5/0x4b0
[12539.283091] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
One way to prevent this deadlock scenario from happening is to not
allow writing to userspace while holding the key semaphore. Instead,
an internal buffer is allocated for getting the keys out from the
read method first before copying them out to userspace without holding
the lock.
That requires taking out the __user modifier from the read methods as
well as additional changes to not use any userspace write helpers.
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
security/keys/big_key.c | 11 ++---
security/keys/encrypted-keys/encrypted.c | 7 ++-
security/keys/keyctl.c | 57 +++++++++++++++++++----
security/keys/keyring.c | 6 +--
security/keys/request_key_auth.c | 7 ++-
security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
security/keys/user_defined.c | 5 +-
10 files changed, 67 insertions(+), 47 deletions(-)
diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index f6a7ba4dccd4..3fee04f81439 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -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 */
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index d5e73266a81a..be61fcddc02a 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-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)
{
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 4ded94bcf274..2ab2d6d6aeab 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -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)
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 001abe530a0d..82008f900930 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -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;
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 60720f58cbe0..f6797ba44bf7 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -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;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..81f68e434b9f 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -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.
*
@@ -844,16 +859,42 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
/* the key is probably readable - now try to read it */
can_read_key:
- ret = -EOPNOTSUPP;
- if (key->type->read) {
- /* Read the data with the semaphore held (since we might sleep)
+ if (!key->type->read) {
+ ret = -EOPNOTSUPP;
+ goto error2;
+ }
+
+ if (!buffer || !buflen) {
+ /* Get the key length from the read method */
+ ret = __keyctl_read_key(key, NULL, 0);
+ } else {
+
+ /*
+ * 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.
+ */
+ char *tmpbuf = kmalloc(buflen, GFP_KERNEL);
+
+ if (!tmpbuf) {
+ ret = -ENOMEM;
+ goto error2;
+ }
+ ret = __keyctl_read_key(key, tmpbuf, buflen);
+
+ /*
+ * Read methods will just return the required length
+ * without any copying if the provided length isn't big
+ * enough.
*/
- down_read(&key->sem);
- ret = key_validate(key);
- if (ret == 0)
- ret = key->type->read(key, buffer, buflen);
- up_read(&key->sem);
+ if ((ret > 0) && (ret <= buflen)) {
+ if (copy_to_user(buffer, tmpbuf, ret))
+ ret = -EFAULT;
+ }
+ kzfree(tmpbuf);
}
error2:
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..5ca620d31cd3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -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;
}
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ecba39c93fd9..41e9735006d0 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -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;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index d2c5ec1e040b..8001ab07e63b 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -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;
}
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 6f12de4ce549..07d4287e9084 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -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;
--
2.18.1
^ permalink raw reply related
* [PATCH v4 3/4] KEYS: Remove __user annotation from dns_resolver_read()
From: Waiman Long @ 2020-03-17 19:41 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200317194140.6031-1-longman@redhat.com>
As the keyctl_read_key() has been modified to use a temporary kernel
buffer to read out the key data instead of passing in the user-supplied
buffer directly, there is no need to use the __user annotation for
dns_resolver_read().
Signed-off-by: Waiman Long <longman@redhat.com>
---
net/dns_resolver/dns_key.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3e1a90669006..ad53eb31d40f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -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]);
--
2.18.1
^ permalink raw reply related
* [PATCH v4 2/4] KEYS: Remove __user annotation from rxrpc_read()
From: Waiman Long @ 2020-03-17 19:41 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200317194140.6031-1-longman@redhat.com>
As the keyctl_read_key() has been modified to use a temporary kernel
buffer to read out the key data instead of passing in the user-supplied
buffer directly, there is no need to use the __user annotation for
rxrpc_read(). In addition,
1) The put_user() call is replaced by a direct copy.
2) The copy_to_user() call is replaced by memcpy().
3) All the fault handling code is removed.
Compiling on a x86-64 system, the size of the rxrpc_read() function is
reduced from 3795 bytes to 2384 bytes with this patch.
Signed-off-by: Waiman Long <longman@redhat.com>
---
net/rxrpc/key.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 6c3f35fac42d..0c98313dd7a8 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -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;
}
--
2.18.1
^ permalink raw reply related
* [PATCH v4 0/4] KEYS: Read keys to internal buffer & then copy to userspace
From: Waiman Long @ 2020-03-17 19:41 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
v4:
- Remove the __user annotation from big_key_read() and user_read() in
patch 1.
- Add a new patch 2 to remove __user annotation from rxrpc_read().
- Add a new patch 3 to remove __user annotation from dns_resolver_read().
- Merge the original patches 2 and 3 into a single patch 4 and refactor
it as suggested by Jarkko and Eric.
v3:
- Reorganize the keyctl_read_key() code to make it more readable as
suggested by Jarkko Sakkinen.
- Add patch 3 to use kvmalloc() for safer large buffer allocation as
suggested by David Howells.
v2:
- Handle NULL buffer and buflen properly in patch 1.
- Fix a bug in big_key.c.
- Add patch 2 to handle arbitrary large user-supplied buflen.
The current security key read methods are called with the key semaphore
held. The methods then copy out the key data to userspace which is
subjected to page fault and may acquire the mmap semaphore. That can
result in circular lock dependency and hence a chance to get into
deadlock.
To avoid such a deadlock, an internal buffer is now allocated for getting
out the necessary data first. After releasing the key semaphore, the
key data are then copied out to userspace sidestepping the circular
lock dependency.
The keyutils test suite was run and the test passed with these patchset
applied without any falure.
Waiman Long (4):
KEYS: Don't write out to userspace while holding key semaphore
KEYS: Remove __user annotation from rxrpc_read()
KEYS: Remove __user annotation from dns_resolver_read()
KEYS: Avoid false positive ENOMEM error on key read
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 +++----
security/keys/big_key.c | 11 ++-
security/keys/encrypted-keys/encrypted.c | 7 +-
security/keys/internal.h | 12 ++++
security/keys/keyctl.c | 86 ++++++++++++++++++++---
security/keys/keyring.c | 6 +-
security/keys/request_key_auth.c | 7 +-
security/keys/trusted-keys/trusted_tpm1.c | 14 +---
security/keys/user_defined.c | 5 +-
13 files changed, 116 insertions(+), 68 deletions(-)
--
2.18.1
^ permalink raw reply
* Re: [PATCH v3 2/3] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-17 18:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200315213245.GF224162@linux.intel.com>
On 3/15/20 5:32 PM, Jarkko Sakkinen wrote:
> On Fri, Mar 13, 2020 at 11:21:01AM -0400, Waiman Long wrote:
>> - * Read methods will just return the required length
>> - * without any copying if the provided length isn't big
>> - * enough.
>> + * We don't want an erronous -ENOMEM error due to an
>> + * arbitrary large user-supplied buflen. So if buflen
>> + * exceeds a threshold (1024 bytes in this case), we call
>> + * the read method twice. The first time to get the buffer
>> + * length and the second time to read out the key data.
>> + *
>> + * N.B. All the read methods will return the required
>> + * buffer length with a NULL input buffer or when
>> + * the input buffer length isn't large enough.
>> */
>> + if (buflen <= 0x400) {
> 1. The overwhelmingly long comment. Will be destined to rotten.
> 2. Magic number.
> 3. The cap must be updated both in comment and code, and not only
> that, but the numbers use a different base (dec and hex).
>
> /Jarkko
>
Thank for the comment. I will make the necessary change.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-17 18:10 UTC (permalink / raw)
To: Jarkko Sakkinen, David Howells
Cc: James Morris, Serge E. Hallyn, Mimi Zohar, keyrings, linux-kernel,
linux-security-module, linux-integrity, Sumit Garg,
Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <c1138c83619553d018970a4b2d95f38fccebc99c.camel@linux.intel.com>
On 3/16/20 9:53 AM, Jarkko Sakkinen wrote:
> On Mon, 2020-03-16 at 11:22 +0000, David Howells wrote:
>> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>>
>>> I guess we cannot sanely define fixes tag for this one, can we?
>> Use:
>>
>> Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
>>
>> David
> Longmao, please include this to the next version.
>
> /Jarkko
>
Sure, will do.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-17 18:09 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <1797278.1584359451@warthog.procyon.org.uk>
On 3/16/20 7:50 AM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> include/linux/key-type.h | 2 +-
>> security/keys/big_key.c | 11 ++---
>> security/keys/encrypted-keys/encrypted.c | 7 ++-
>> security/keys/keyctl.c | 57 +++++++++++++++++++----
>> security/keys/keyring.c | 6 +--
>> security/keys/request_key_auth.c | 7 ++-
>> security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
>> security/keys/user_defined.c | 5 +-
>> ...
>> - long (*read)(const struct key *key, char __user *buffer, size_t buflen);
>> + long (*read)(const struct key *key, char *buffer, size_t buflen);
> Note that there are read functions outside of security/keys/ that also need
> fixing - dns_resolver_read() and rxrpc_read().
>
> David
Yes, I am going to fix that. Sorry for the delay as I am juggling a few
different tasks.
Cheers,
Longman
^ permalink raw reply
* Re: [RFC PATCH v14 06/10] landlock: Add syscall implementation
From: Mickaël Salaün @ 2020-03-17 17:51 UTC (permalink / raw)
To: Al Viro
Cc: linux-kernel, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Greg Kroah-Hartman, James Morris, Jann Horn, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün,
Serge E . Hallyn, Shuah Khan, Vincent Dagonneau, kernel-hardening,
linux-api, linux-arch, linux-doc, linux-fsdevel, linux-kselftest,
linux-security-module, x86
In-Reply-To: <20200317164709.GA23230@ZenIV.linux.org.uk>
On 17/03/2020 17:47, Al Viro wrote:
> On Mon, Feb 24, 2020 at 05:02:11PM +0100, Mickaël Salaün wrote:
>
>> +static int get_path_from_fd(u64 fd, struct path *path)
>
>> + /*
>> + * Only allows O_PATH FD: enable to restrict ambiant (FS) accesses
>> + * without requiring to open and risk leaking or misuing a FD. Accept
>> + * removed, but still open directory (S_DEAD).
>> + */
>> + if (!(f.file->f_mode & FMODE_PATH) || !f.file->f_path.mnt ||
> ^^^^^^^^^^^^^^^^^^^
> Could you explain what that one had been be about? The underlined
> subexpression is always false; was that supposed to check some
> condition and if so, which one?
>
This was just to be sure that the next assignment "path->mnt =
f.file->f_path.mnt;" always creates a valid path. If this is always
true, I will remove it.
^ permalink raw reply
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: Mickaël Salaün @ 2020-03-17 17:50 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <CAG48ez0=0W5Ok-8nASqZrZ28JboXRRi3gDxV5u6mdcOtzwuRVA@mail.gmail.com>
On 17/03/2020 17:19, Jann Horn wrote:
> On Thu, Mar 12, 2020 at 12:38 AM Mickaël Salaün <mic@digikod.net> wrote:
>> On 10/03/2020 00:44, Jann Horn wrote:
>>> On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
[...]
>>> Aside from those things, there is also a major correctness issue where
>>> I'm not sure how to solve it properly:
>>>
>>> Let's say a process installs a filter on itself like this:
>>>
>>> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
>>> ACCESS_FS_ROUGHLY_WRITE};
>>> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
>>> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
>>> struct landlock_attr_path_beneath path_beneath = {
>>> .ruleset_fd = ruleset_fd,
>>> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
>>> .parent_fd = open("/tmp/foobar", O_PATH),
>>> };
>>> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
>>> sizeof(path_beneath), &path_beneath);
>>> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>>> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
>>> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
>>> sizeof(attr_enforce), &attr_enforce);
>>>
>>> At this point, the process is not supposed to be able to write to
>>> anything outside /tmp/foobar, right? But what happens if the process
>>> does the following next?
>>>
>>> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
>>> ACCESS_FS_ROUGHLY_WRITE};
>>> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
>>> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
>>> struct landlock_attr_path_beneath path_beneath = {
>>> .ruleset_fd = ruleset_fd,
>>> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
>>> .parent_fd = open("/", O_PATH),
>>> };
>>> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
>>> sizeof(path_beneath), &path_beneath);
>>> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>>> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
>>> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
>>> sizeof(attr_enforce), &attr_enforce);
>>>
>>> As far as I can tell from looking at the source, after this, you will
>>> have write access to the entire filesystem again. I think the idea is
>>> that LANDLOCK_CMD_ENFORCE_RULESET should only let you drop privileges,
>>> not increase them, right?
>>
>> There is an additionnal check in syscall.c:get_path_from_fd(): it is
>> forbidden to add a rule with a path which is not accessible (according
>> to LANDLOCK_ACCESS_FS_OPEN) thanks to a call to security_file_open(),
>> but this is definitely not perfect.
>
> Ah, I missed that.
>
>>> I think the easy way to fix this would be to add a bitmask to each
>>> rule that says from which ruleset it originally comes, and then let
>>> check_access_path() collect these bitmasks from each rule with OR, and
>>> check at the end whether the resulting bitmask is full - if not, at
>>> least one of the rulesets did not permit the access, and it should be
>>> denied.
>>>
>>> But maybe it would make more sense to change how the API works
>>> instead, and get rid of the concept of "merging" two rulesets
>>> together? Instead, we could make the API work like this:
>>>
>>> - LANDLOCK_CMD_CREATE_RULESET gives you a file descriptor whose
>>> ->private_data contains a pointer to the old ruleset of the process,
>>> as well as a pointer to a new empty ruleset.
>>> - LANDLOCK_CMD_ADD_RULE fails if the specified rule would not be
>>> permitted by the old ruleset, then adds the rule to the new ruleset
>>> - LANDLOCK_CMD_ENFORCE_RULESET fails if the old ruleset pointer in
>>> ->private_data doesn't match the current ruleset of the process, then
>>> replaces the old ruleset with the new ruleset.
>>>
>>> With this, the new ruleset is guaranteed to be a subset of the old
>>> ruleset because each of the new ruleset's rules is permitted by the
>>> old ruleset. (Unless the directory hierarchy rotates, but in that case
>>> the inaccuracy isn't much worse than what would've been possible
>>> through RCU path walk anyway AFAIK.)
>>>
>>> What do you think?
>>>
>>
>> I would prefer to add the same checks you described at first (with
>> check_access_path), but only when creating a new ruleset with
>> merge_ruleset() (which should probably be renamed). This enables not to
>> rely on a parent ruleset/domain until the enforcement, which is the case
>> anyway.
>> Unfortunately this doesn't work for some cases with bind mounts. Because
>> check_access_path() goes through one path, another (bind mounted) path
>> could be illegitimately allowed.
>
> Hmm... I'm not sure what you mean. At the moment, landlock doesn't
> allow any sandboxed process to change the mount hierarchy, right? Can
> you give an example where this would go wrong?
Indeed, a Landlocked process must no be able to change its mount
namespace layout. However, bind mounts may already exist.
Let's say a process sandbox itself to only access /a in a read-write
way. Then, this process (or one of its children) add a new restriction
on /a/b to only be able to read this hierarchy. The check at insertion
time would allow this because this access right is a subset of the
access right allowed with the parent directory. However, If /a/b is bind
mounted somewhere else, let's say in /private/b, then the second
enforcement just gave new access rights to this hierarchy too. This is
why it seems risky to rely on a check about the legitimacy of a new
access right when adding it to a ruleset or when enforcing it.
>
>> That makes the problem a bit more complicated. A solution may be to keep
>> track of the hierarchy of each rule (e.g. with a layer/depth number),
>> and only allow an access request if at least a rule of each layer allow
>> this access. In this case we also need to correctly handle the case when
>> rules from different layers are tied to the same object.
>
^ permalink raw reply
* Re: [RFC PATCH v14 06/10] landlock: Add syscall implementation
From: Al Viro @ 2020-03-17 16:47 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Greg Kroah-Hartman, James Morris, Jann Horn, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün,
Serge E . Hallyn, Shuah Khan, Vincent Dagonneau, kernel-hardening,
linux-api, linux-arch, linux-doc, linux-fsdevel, linux-kselftest,
linux-security-module, x86
In-Reply-To: <20200224160215.4136-7-mic@digikod.net>
On Mon, Feb 24, 2020 at 05:02:11PM +0100, Mickaël Salaün wrote:
> +static int get_path_from_fd(u64 fd, struct path *path)
> + /*
> + * Only allows O_PATH FD: enable to restrict ambiant (FS) accesses
> + * without requiring to open and risk leaking or misuing a FD. Accept
> + * removed, but still open directory (S_DEAD).
> + */
> + if (!(f.file->f_mode & FMODE_PATH) || !f.file->f_path.mnt ||
^^^^^^^^^^^^^^^^^^^
Could you explain what that one had been be about? The underlined
subexpression is always false; was that supposed to check some
condition and if so, which one?
^ permalink raw reply
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: Jann Horn @ 2020-03-17 16:19 UTC (permalink / raw)
To: Mickaël Salaün
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <873d7419-bdd9-8a52-0a9b-dddbe31df4f9@digikod.net>
On Thu, Mar 12, 2020 at 12:38 AM Mickaël Salaün <mic@digikod.net> wrote:
> On 10/03/2020 00:44, Jann Horn wrote:
> > On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
> >> This new version of Landlock is a major revamp of the previous series
> >> [1], hence the RFC tag. The three main changes are the replacement of
> >> eBPF with a dedicated safe management of access rules, the replacement
> >> of the use of seccomp(2) with a dedicated syscall, and the management of
> >> filesystem access-control (back from the v10).
> >>
> >> As discussed in [2], eBPF may be too powerful and dangerous to be put in
> >> the hand of unprivileged and potentially malicious processes, especially
> >> because of side-channel attacks against access-controls or other parts
> >> of the kernel.
> >>
> >> Thanks to this new implementation (1540 SLOC), designed from the ground
> >> to be used by unprivileged processes, this series enables a process to
> >> sandbox itself without requiring CAP_SYS_ADMIN, but only the
> >> no_new_privs constraint (like seccomp). Not relying on eBPF also
> >> enables to improve performances, especially for stacked security
> >> policies thanks to mergeable rulesets.
> >>
> >> The compiled documentation is available here:
> >> https://landlock.io/linux-doc/landlock-v14/security/landlock/index.html
> >>
> >> This series can be applied on top of v5.6-rc3. This can be tested with
> >> CONFIG_SECURITY_LANDLOCK and CONFIG_SAMPLE_LANDLOCK. This patch series
> >> can be found in a Git repository here:
> >> https://github.com/landlock-lsm/linux/commits/landlock-v14
> >> I would really appreciate constructive comments on the design and the code.
> >
> > I've looked through the patchset, and I think that it would be
> > possible to simplify it quite a bit. I have tried to do that (and
> > compiled-tested it, but not actually tried running it); here's what I
> > came up with:
> >
> > https://github.com/thejh/linux/commits/landlock-mod
> >
> > The three modified patches (patches 1, 2 and 5) are marked with
> > "[MODIFIED]" in their title. Please take a look - what do you think?
> > Feel free to integrate my changes into your patches if you think they
> > make sense.
>
> Regarding the landlock_release_inodes(), the final wait_var_event() is
> indeed needed (as does fsnotify), but why do you use a READ_ONCE() for
> landlock_initialized?
Ah, good point - that READ_ONCE() should be unnecessary.
> The other main change is about the object cross-reference: you entirely
> removed it, which means that an object will only be free when there are
> no rules using it. This does not free an object when its underlying
> object is being terminated. We now only have to worry about the
> termination of the parent of an underlying object (e.g. the super-block
> of an inode).
>
> However, I think you forgot to increment object->usage in
> create_ruleset_elem().
Whoops, you're right.
> There is also an unused checked_mask variable in merge_ruleset().
Oh, yeah, oops.
> All this removes optimizations that made the code more difficult to
> understand. The performance difference is negligible, and I think that
> the memory footprint is fine.
> These optimizations (and others) could be discussed later. I'm
> integrating most of your changes in the next patch series.
:)
> > Aside from those things, there is also a major correctness issue where
> > I'm not sure how to solve it properly:
> >
> > Let's say a process installs a filter on itself like this:
> >
> > struct landlock_attr_ruleset ruleset = { .handled_access_fs =
> > ACCESS_FS_ROUGHLY_WRITE};
> > int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
> > LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
> > struct landlock_attr_path_beneath path_beneath = {
> > .ruleset_fd = ruleset_fd,
> > .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
> > .parent_fd = open("/tmp/foobar", O_PATH),
> > };
> > landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
> > sizeof(path_beneath), &path_beneath);
> > prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> > struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
> > landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
> > sizeof(attr_enforce), &attr_enforce);
> >
> > At this point, the process is not supposed to be able to write to
> > anything outside /tmp/foobar, right? But what happens if the process
> > does the following next?
> >
> > struct landlock_attr_ruleset ruleset = { .handled_access_fs =
> > ACCESS_FS_ROUGHLY_WRITE};
> > int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
> > LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
> > struct landlock_attr_path_beneath path_beneath = {
> > .ruleset_fd = ruleset_fd,
> > .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
> > .parent_fd = open("/", O_PATH),
> > };
> > landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
> > sizeof(path_beneath), &path_beneath);
> > prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> > struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
> > landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
> > sizeof(attr_enforce), &attr_enforce);
> >
> > As far as I can tell from looking at the source, after this, you will
> > have write access to the entire filesystem again. I think the idea is
> > that LANDLOCK_CMD_ENFORCE_RULESET should only let you drop privileges,
> > not increase them, right?
>
> There is an additionnal check in syscall.c:get_path_from_fd(): it is
> forbidden to add a rule with a path which is not accessible (according
> to LANDLOCK_ACCESS_FS_OPEN) thanks to a call to security_file_open(),
> but this is definitely not perfect.
Ah, I missed that.
> > I think the easy way to fix this would be to add a bitmask to each
> > rule that says from which ruleset it originally comes, and then let
> > check_access_path() collect these bitmasks from each rule with OR, and
> > check at the end whether the resulting bitmask is full - if not, at
> > least one of the rulesets did not permit the access, and it should be
> > denied.
> >
> > But maybe it would make more sense to change how the API works
> > instead, and get rid of the concept of "merging" two rulesets
> > together? Instead, we could make the API work like this:
> >
> > - LANDLOCK_CMD_CREATE_RULESET gives you a file descriptor whose
> > ->private_data contains a pointer to the old ruleset of the process,
> > as well as a pointer to a new empty ruleset.
> > - LANDLOCK_CMD_ADD_RULE fails if the specified rule would not be
> > permitted by the old ruleset, then adds the rule to the new ruleset
> > - LANDLOCK_CMD_ENFORCE_RULESET fails if the old ruleset pointer in
> > ->private_data doesn't match the current ruleset of the process, then
> > replaces the old ruleset with the new ruleset.
> >
> > With this, the new ruleset is guaranteed to be a subset of the old
> > ruleset because each of the new ruleset's rules is permitted by the
> > old ruleset. (Unless the directory hierarchy rotates, but in that case
> > the inaccuracy isn't much worse than what would've been possible
> > through RCU path walk anyway AFAIK.)
> >
> > What do you think?
> >
>
> I would prefer to add the same checks you described at first (with
> check_access_path), but only when creating a new ruleset with
> merge_ruleset() (which should probably be renamed). This enables not to
> rely on a parent ruleset/domain until the enforcement, which is the case
> anyway.
> Unfortunately this doesn't work for some cases with bind mounts. Because
> check_access_path() goes through one path, another (bind mounted) path
> could be illegitimately allowed.
Hmm... I'm not sure what you mean. At the moment, landlock doesn't
allow any sandboxed process to change the mount hierarchy, right? Can
you give an example where this would go wrong?
> That makes the problem a bit more complicated. A solution may be to keep
> track of the hierarchy of each rule (e.g. with a layer/depth number),
> and only allow an access request if at least a rule of each layer allow
> this access. In this case we also need to correctly handle the case when
> rules from different layers are tied to the same object.
^ permalink raw reply
* Re: [PATCH v3 3/3] KEYS: Use kvmalloc() to better handle large buffer allocation
From: Jarkko Sakkinen @ 2020-03-16 22:19 UTC (permalink / raw)
To: David Howells, Waiman Long
Cc: James Morris, Serge E. Hallyn, Mimi Zohar, keyrings, linux-kernel,
linux-security-module, linux-integrity, Sumit Garg,
Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <1809107.1584368672@warthog.procyon.org.uk>
On Mon, 2020-03-16 at 14:24 +0000, David Howells wrote:
> I wonder if it's worth merging this into patch 2. I'm not sure it's really
> worth its own patch. If you want to generalise kvzfree(), then that could go
> as its own patch first.
>
> David
Also in the sense that there is no senseful situation where you'd
only wanted to pick either but not both.
/Jarkko
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-16 16:33 UTC (permalink / raw)
To: Jarkko Sakkinen, David Howells
Cc: James Morris, Serge E. Hallyn, Mimi Zohar, keyrings, linux-kernel,
linux-security-module, linux-integrity, Sumit Garg,
Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <c1138c83619553d018970a4b2d95f38fccebc99c.camel@linux.intel.com>
On 3/16/20 9:53 AM, Jarkko Sakkinen wrote:
> On Mon, 2020-03-16 at 11:22 +0000, David Howells wrote:
>> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>>
>>> I guess we cannot sanely define fixes tag for this one, can we?
>> Use:
>>
>> Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
>>
>> David
> Longmao, please include this to the next version.
>
> /Jarkko
>
Sure, will do.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v3 3/3] KEYS: Use kvmalloc() to better handle large buffer allocation
From: Waiman Long @ 2020-03-16 15:21 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <1809107.1584368672@warthog.procyon.org.uk>
On 3/16/20 10:24 AM, David Howells wrote:
> I wonder if it's worth merging this into patch 2. I'm not sure it's really
> worth its own patch. If you want to generalise kvzfree(), then that could go
> as its own patch first.
>
> David
Sure, I can merge it into patch 2.
Thanks,
Longman
^ permalink raw reply
* Re: [PATCH v3 3/3] KEYS: Use kvmalloc() to better handle large buffer allocation
From: David Howells @ 2020-03-16 14:24 UTC (permalink / raw)
To: Waiman Long
Cc: dhowells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, keyrings, linux-kernel, linux-security-module,
linux-integrity, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen
In-Reply-To: <20200313152102.1707-4-longman@redhat.com>
I wonder if it's worth merging this into patch 2. I'm not sure it's really
worth its own patch. If you want to generalise kvzfree(), then that could go
as its own patch first.
David
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: Jarkko Sakkinen @ 2020-03-16 13:53 UTC (permalink / raw)
To: David Howells
Cc: Waiman Long, James Morris, Serge E. Hallyn, Mimi Zohar, keyrings,
linux-kernel, linux-security-module, linux-integrity, Sumit Garg,
Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <1793253.1584357764@warthog.procyon.org.uk>
On Mon, 2020-03-16 at 11:22 +0000, David Howells wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
> > I guess we cannot sanely define fixes tag for this one, can we?
>
> Use:
>
> Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
>
> David
Longmao, please include this to the next version.
/Jarkko
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: David Howells @ 2020-03-16 11:50 UTC (permalink / raw)
To: Waiman Long
Cc: dhowells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, keyrings, linux-kernel, linux-security-module,
linux-integrity, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen
In-Reply-To: <20200313152102.1707-2-longman@redhat.com>
Waiman Long <longman@redhat.com> wrote:
> include/linux/key-type.h | 2 +-
> security/keys/big_key.c | 11 ++---
> security/keys/encrypted-keys/encrypted.c | 7 ++-
> security/keys/keyctl.c | 57 +++++++++++++++++++----
> security/keys/keyring.c | 6 +--
> security/keys/request_key_auth.c | 7 ++-
> security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
> security/keys/user_defined.c | 5 +-
> ...
> - long (*read)(const struct key *key, char __user *buffer, size_t buflen);
> + long (*read)(const struct key *key, char *buffer, size_t buflen);
Note that there are read functions outside of security/keys/ that also need
fixing - dns_resolver_read() and rxrpc_read().
David
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: David Howells @ 2020-03-16 11:34 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dhowells, Waiman Long, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200315212706.GE224162@linux.intel.com>
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> Do you have a test case that can reproduce this on a constant basis?
In this case it's quite tricky because a network filesystem is involved. I
wonder if it might be possible to do it with fscrypt or ecryptfs.
David
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: David Howells @ 2020-03-16 11:22 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dhowells, Waiman Long, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200315212706.GE224162@linux.intel.com>
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> I guess we cannot sanely define fixes tag for this one, can we?
Use:
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
David
^ permalink raw reply
* Re: [PATCH v3 3/3] KEYS: Use kvmalloc() to better handle large buffer allocation
From: Waiman Long @ 2020-03-15 22:01 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Eric Biggers, David Howells, James Morris, Serge E. Hallyn,
Mimi Zohar, keyrings, linux-kernel, linux-security-module,
linux-integrity, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Chris von Recklinghausen
In-Reply-To: <20200315215253.GG224162@linux.intel.com>
On 3/15/20 5:52 PM, Jarkko Sakkinen wrote:
> On Fri, Mar 13, 2020 at 01:49:57PM -0400, Waiman Long wrote:
>>>> if (!tmpbuf || unlikely(ret > tmpbuflen)) {
>>>> if (unlikely(tmpbuf))
>>>> - kzfree(tmpbuf);
>>>> + __kvzfree(tmpbuf, tmpbuflen);
>>> Both kzfree() and __kvzfree() handle a NULL pointer, so there's no need for the
>>> NULL check first.
>>>
>> I would like to keep this one because of the unlikely annotation.
> What (measurable) gain does it bring anyway?
It is not a performance issue. I just want to indicate that the need to
free should not happen at all. It match the unlikely tag in the if
condition above.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v3 3/3] KEYS: Use kvmalloc() to better handle large buffer allocation
From: Jarkko Sakkinen @ 2020-03-15 21:52 UTC (permalink / raw)
To: Waiman Long
Cc: Eric Biggers, David Howells, James Morris, Serge E. Hallyn,
Mimi Zohar, keyrings, linux-kernel, linux-security-module,
linux-integrity, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Chris von Recklinghausen
In-Reply-To: <8f2f1787-88b0-f86d-991c-34cfd2f9b4aa@redhat.com>
On Fri, Mar 13, 2020 at 01:49:57PM -0400, Waiman Long wrote:
> >> if (!tmpbuf || unlikely(ret > tmpbuflen)) {
> >> if (unlikely(tmpbuf))
> >> - kzfree(tmpbuf);
> >> + __kvzfree(tmpbuf, tmpbuflen);
> > Both kzfree() and __kvzfree() handle a NULL pointer, so there's no need for the
> > NULL check first.
> >
> I would like to keep this one because of the unlikely annotation.
What (measurable) gain does it bring anyway?
/Jarkko
^ permalink raw reply
* Re: [PATCH v3 2/3] KEYS: Avoid false positive ENOMEM error on key read
From: Jarkko Sakkinen @ 2020-03-15 21:32 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200313152102.1707-3-longman@redhat.com>
On Fri, Mar 13, 2020 at 11:21:01AM -0400, Waiman Long wrote:
> - * Read methods will just return the required length
> - * without any copying if the provided length isn't big
> - * enough.
> + * We don't want an erronous -ENOMEM error due to an
> + * arbitrary large user-supplied buflen. So if buflen
> + * exceeds a threshold (1024 bytes in this case), we call
> + * the read method twice. The first time to get the buffer
> + * length and the second time to read out the key data.
> + *
> + * N.B. All the read methods will return the required
> + * buffer length with a NULL input buffer or when
> + * the input buffer length isn't large enough.
> */
> + if (buflen <= 0x400) {
1. The overwhelmingly long comment. Will be destined to rotten.
2. Magic number.
3. The cap must be updated both in comment and code, and not only
that, but the numbers use a different base (dec and hex).
/Jarkko
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: Jarkko Sakkinen @ 2020-03-15 21:27 UTC (permalink / raw)
To: Waiman Long, David Howells
Cc: James Morris, Serge E. Hallyn, Mimi Zohar, keyrings, linux-kernel,
linux-security-module, linux-integrity, Sumit Garg,
Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200315192104.GD224162@linux.intel.com>
On Sun, Mar 15, 2020 at 09:21:16PM +0200, Jarkko Sakkinen wrote:
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
I guess we cannot sanely define fixes tag for this one, can we?
Surely this should still go to stable.
Do you have a test case that can reproduce this on a constant basis?
/Jarkko
^ permalink raw reply
* Re: [PATCH v3 1/3] KEYS: Don't write out to userspace while holding key semaphore
From: Jarkko Sakkinen @ 2020-03-15 19:21 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200313152102.1707-2-longman@redhat.com>
On Fri, Mar 13, 2020 at 11:21:00AM -0400, Waiman Long wrote:
> A lockdep circular locking dependency report was seen when running a
> keyutils test:
>
> [12537.027242] ======================================================
> [12537.059309] WARNING: possible circular locking dependency detected
> [12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE --------- - -
> [12537.125253] ------------------------------------------------------
> [12537.153189] keyctl/25598 is trying to acquire lock:
> [12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
> [12537.208365]
> [12537.208365] but task is already holding lock:
> [12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
> [12537.270476]
> [12537.270476] which lock already depends on the new lock.
> [12537.270476]
> [12537.307209]
> [12537.307209] the existing dependency chain (in reverse order) is:
> [12537.340754]
> [12537.340754] -> #3 (&type->lock_class){++++}:
> [12537.367434] down_write+0x4d/0x110
> [12537.385202] __key_link_begin+0x87/0x280
> [12537.405232] request_key_and_link+0x483/0xf70
> [12537.427221] request_key+0x3c/0x80
> [12537.444839] dns_query+0x1db/0x5a5 [dns_resolver]
> [12537.468445] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
> [12537.496731] cifs_reconnect+0xe04/0x2500 [cifs]
> [12537.519418] cifs_readv_from_socket+0x461/0x690 [cifs]
> [12537.546263] cifs_read_from_socket+0xa0/0xe0 [cifs]
> [12537.573551] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
> [12537.601045] kthread+0x30c/0x3d0
> [12537.617906] ret_from_fork+0x3a/0x50
> [12537.636225]
> [12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
> [12537.664525] __mutex_lock+0x105/0x11f0
> [12537.683734] request_key_and_link+0x35a/0xf70
> [12537.705640] request_key+0x3c/0x80
> [12537.723304] dns_query+0x1db/0x5a5 [dns_resolver]
> [12537.746773] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
> [12537.775607] cifs_reconnect+0xe04/0x2500 [cifs]
> [12537.798322] cifs_readv_from_socket+0x461/0x690 [cifs]
> [12537.823369] cifs_read_from_socket+0xa0/0xe0 [cifs]
> [12537.847262] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
> [12537.873477] kthread+0x30c/0x3d0
> [12537.890281] ret_from_fork+0x3a/0x50
> [12537.908649]
> [12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
> [12537.935225] __mutex_lock+0x105/0x11f0
> [12537.954450] cifs_call_async+0x102/0x7f0 [cifs]
> [12537.977250] smb2_async_readv+0x6c3/0xc90 [cifs]
> [12538.000659] cifs_readpages+0x120a/0x1e50 [cifs]
> [12538.023920] read_pages+0xf5/0x560
> [12538.041583] __do_page_cache_readahead+0x41d/0x4b0
> [12538.067047] ondemand_readahead+0x44c/0xc10
> [12538.092069] filemap_fault+0xec1/0x1830
> [12538.111637] __do_fault+0x82/0x260
> [12538.129216] do_fault+0x419/0xfb0
> [12538.146390] __handle_mm_fault+0x862/0xdf0
> [12538.167408] handle_mm_fault+0x154/0x550
> [12538.187401] __do_page_fault+0x42f/0xa60
> [12538.207395] do_page_fault+0x38/0x5e0
> [12538.225777] page_fault+0x1e/0x30
> [12538.243010]
> [12538.243010] -> #0 (&mm->mmap_sem){++++}:
> [12538.267875] lock_acquire+0x14c/0x420
> [12538.286848] __might_fault+0x119/0x1b0
> [12538.306006] keyring_read_iterator+0x7e/0x170
> [12538.327936] assoc_array_subtree_iterate+0x97/0x280
> [12538.352154] keyring_read+0xe9/0x110
> [12538.370558] keyctl_read_key+0x1b9/0x220
> [12538.391470] do_syscall_64+0xa5/0x4b0
> [12538.410511] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
> [12538.435535]
> [12538.435535] other info that might help us debug this:
> [12538.435535]
> [12538.472829] Chain exists of:
> [12538.472829] &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
> [12538.472829]
> [12538.524820] Possible unsafe locking scenario:
> [12538.524820]
> [12538.551431] CPU0 CPU1
> [12538.572654] ---- ----
> [12538.595865] lock(&type->lock_class);
> [12538.613737] lock(root_key_user.cons_lock);
> [12538.644234] lock(&type->lock_class);
> [12538.672410] lock(&mm->mmap_sem);
> [12538.687758]
> [12538.687758] *** DEADLOCK ***
> [12538.687758]
> [12538.714455] 1 lock held by keyctl/25598:
> [12538.732097] #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
> [12538.770573]
> [12538.770573] stack backtrace:
> [12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
> [12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
> [12538.881963] Call Trace:
> [12538.892897] dump_stack+0x9a/0xf0
> [12538.907908] print_circular_bug.isra.25.cold.50+0x1bc/0x279
> [12538.932891] ? save_trace+0xd6/0x250
> [12538.948979] check_prev_add.constprop.32+0xc36/0x14f0
> [12538.971643] ? keyring_compare_object+0x104/0x190
> [12538.992738] ? check_usage+0x550/0x550
> [12539.009845] ? sched_clock+0x5/0x10
> [12539.025484] ? sched_clock_cpu+0x18/0x1e0
> [12539.043555] __lock_acquire+0x1f12/0x38d0
> [12539.061551] ? trace_hardirqs_on+0x10/0x10
> [12539.080554] lock_acquire+0x14c/0x420
> [12539.100330] ? __might_fault+0xc4/0x1b0
> [12539.119079] __might_fault+0x119/0x1b0
> [12539.135869] ? __might_fault+0xc4/0x1b0
> [12539.153234] keyring_read_iterator+0x7e/0x170
> [12539.172787] ? keyring_read+0x110/0x110
> [12539.190059] assoc_array_subtree_iterate+0x97/0x280
> [12539.211526] keyring_read+0xe9/0x110
> [12539.227561] ? keyring_gc_check_iterator+0xc0/0xc0
> [12539.249076] keyctl_read_key+0x1b9/0x220
> [12539.266660] do_syscall_64+0xa5/0x4b0
> [12539.283091] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
>
> One way to prevent this deadlock scenario from happening is to not
> allow writing to userspace while holding the key semaphore. Instead,
> an internal buffer is allocated for getting the keys out from the
> read method first before copying them out to userspace without holding
> the lock.
>
> That requires taking out the __user modifier from the read methods as
> well as additional changes to not use any userspace write helpers.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> include/linux/key-type.h | 2 +-
> security/keys/big_key.c | 11 ++---
> security/keys/encrypted-keys/encrypted.c | 7 ++-
> security/keys/keyctl.c | 57 +++++++++++++++++++----
> security/keys/keyring.c | 6 +--
> security/keys/request_key_auth.c | 7 ++-
> security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
> security/keys/user_defined.c | 5 +-
> 8 files changed, 65 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/key-type.h b/include/linux/key-type.h
> index 4ded94bcf274..2ab2d6d6aeab 100644
> --- a/include/linux/key-type.h
> +++ b/include/linux/key-type.h
> @@ -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)
> diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> index 001abe530a0d..82008f900930 100644
> --- a/security/keys/big_key.c
> +++ b/security/keys/big_key.c
> @@ -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;
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index 60720f58cbe0..f6797ba44bf7 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -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;
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 9b898c969558..81f68e434b9f 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -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.
> *
> @@ -844,16 +859,42 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>
> /* the key is probably readable - now try to read it */
> can_read_key:
> - ret = -EOPNOTSUPP;
> - if (key->type->read) {
> - /* Read the data with the semaphore held (since we might sleep)
> + if (!key->type->read) {
> + ret = -EOPNOTSUPP;
> + goto error2;
> + }
> +
> + if (!buffer || !buflen) {
> + /* Get the key length from the read method */
> + ret = __keyctl_read_key(key, NULL, 0);
> + } else {
> +
> + /*
> + * 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.
> + */
> + char *tmpbuf = kmalloc(buflen, GFP_KERNEL);
> +
> + if (!tmpbuf) {
> + ret = -ENOMEM;
> + goto error2;
> + }
> + ret = __keyctl_read_key(key, tmpbuf, buflen);
> +
> + /*
> + * Read methods will just return the required length
> + * without any copying if the provided length isn't big
> + * enough.
> */
> - down_read(&key->sem);
> - ret = key_validate(key);
> - if (ret == 0)
> - ret = key->type->read(key, buffer, buflen);
> - up_read(&key->sem);
> + if ((ret > 0) && (ret <= buflen)) {
> + if (copy_to_user(buffer, tmpbuf, ret))
> + ret = -EFAULT;
> + }
> + kzfree(tmpbuf);
> }
>
> error2:
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index febf36c6ddc5..5ca620d31cd3 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -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;
> }
> diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
> index ecba39c93fd9..41e9735006d0 100644
> --- a/security/keys/request_key_auth.c
> +++ b/security/keys/request_key_auth.c
> @@ -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;
> diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> index d2c5ec1e040b..8001ab07e63b 100644
> --- a/security/keys/trusted-keys/trusted_tpm1.c
> +++ b/security/keys/trusted-keys/trusted_tpm1.c
> @@ -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;
> }
> diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
> index 6f12de4ce549..07d4287e9084 100644
> --- a/security/keys/user_defined.c
> +++ b/security/keys/user_defined.c
> @@ -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;
> --
> 2.18.1
>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* [PATCH v9 1/8] proc: rename struct proc_fs_info to proc_fs_opts
From: Alexey Gladkov @ 2020-03-15 15:25 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov
In-Reply-To: <cover.1584285253.git.gladkov.alexey@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc_namespace.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 273ee82d8aa9..9a8b624bc3db 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -37,23 +37,23 @@ static __poll_t mounts_poll(struct file *file, poll_table *wait)
return res;
}
-struct proc_fs_info {
+struct proc_fs_opts {
int flag;
const char *str;
};
static int show_sb_opts(struct seq_file *m, struct super_block *sb)
{
- static const struct proc_fs_info fs_info[] = {
+ static const struct proc_fs_opts fs_opts[] = {
{ SB_SYNCHRONOUS, ",sync" },
{ SB_DIRSYNC, ",dirsync" },
{ SB_MANDLOCK, ",mand" },
{ SB_LAZYTIME, ",lazytime" },
{ 0, NULL }
};
- const struct proc_fs_info *fs_infop;
+ const struct proc_fs_opts *fs_infop;
- for (fs_infop = fs_info; fs_infop->flag; fs_infop++) {
+ for (fs_infop = fs_opts; fs_infop->flag; fs_infop++) {
if (sb->s_flags & fs_infop->flag)
seq_puts(m, fs_infop->str);
}
@@ -63,7 +63,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb)
static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
{
- static const struct proc_fs_info mnt_info[] = {
+ static const struct proc_fs_opts mnt_opts[] = {
{ MNT_NOSUID, ",nosuid" },
{ MNT_NODEV, ",nodev" },
{ MNT_NOEXEC, ",noexec" },
@@ -72,9 +72,9 @@ static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
{ MNT_RELATIME, ",relatime" },
{ 0, NULL }
};
- const struct proc_fs_info *fs_infop;
+ const struct proc_fs_opts *fs_infop;
- for (fs_infop = mnt_info; fs_infop->flag; fs_infop++) {
+ for (fs_infop = mnt_opts; fs_infop->flag; fs_infop++) {
if (mnt->mnt_flags & fs_infop->flag)
seq_puts(m, fs_infop->str);
}
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox