Skip to content

Commit d3ec10a

Browse files
Waiman-Longdhowells
Waiman-Long
authored and
dhowells
committed
KEYS: Don't write out to userspace while holding key semaphore
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 Freescale#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] -> Freescale#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] -> Freescale#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] -> Freescale#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 all the relevant read methods as well as additional changes to not use any userspace write helpers. That is, 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. Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2") Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Signed-off-by: Waiman Long <longman@redhat.com> Signed-off-by: David Howells <dhowells@redhat.com>
1 parent 1b649e0 commit d3ec10a

File tree

12 files changed

+85
-74
lines changed

12 files changed

+85
-74
lines changed

‎include/keys/big_key-type.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,6 @@ extern void big_key_free_preparse(struct key_preparsed_payload *prep);
1717
extern void big_key_revoke(struct key *key);
1818
extern void big_key_destroy(struct key *key);
1919
extern void big_key_describe(const struct key *big_key, struct seq_file *m);
20-
extern long big_key_read(const struct key *key, char __user *buffer, size_t buflen);
20+
extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
2121

2222
#endif /* _KEYS_BIG_KEY_TYPE_H */

‎include/keys/user-type.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,7 @@ extern int user_update(struct key *key, struct key_preparsed_payload *prep);
4141
extern void user_revoke(struct key *key);
4242
extern void user_destroy(struct key *key);
4343
extern void user_describe(const struct key *user, struct seq_file *m);
44-
extern long user_read(const struct key *key,
45-
char __user *buffer, size_t buflen);
44+
extern long user_read(const struct key *key, char *buffer, size_t buflen);
4645

4746
static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key)
4847
{

‎include/linux/key-type.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ struct key_type {
127127
* much is copied into the buffer
128128
* - shouldn't do the copy if the buffer is NULL
129129
*/
130-
long (*read)(const struct key *key, char __user *buffer, size_t buflen);
130+
long (*read)(const struct key *key, char *buffer, size_t buflen);
131131

132132
/* handle request_key() for this type instead of invoking
133133
* /sbin/request-key (optional)

‎net/dns_resolver/dns_key.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
302302
* - the key's semaphore is read-locked
303303
*/
304304
static long dns_resolver_read(const struct key *key,
305-
char __user *buffer, size_t buflen)
305+
char *buffer, size_t buflen)
306306
{
307307
int err = PTR_ERR(key->payload.data[dns_key_error]);
308308

‎net/rxrpc/key.c‎

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ static void rxrpc_free_preparse_s(struct key_preparsed_payload *);
3131
static void rxrpc_destroy(struct key *);
3232
static void rxrpc_destroy_s(struct key *);
3333
static void rxrpc_describe(const struct key *, struct seq_file *);
34-
static long rxrpc_read(const struct key *, char __user *, size_t);
34+
static long rxrpc_read(const struct key *, char *, size_t);
3535

3636
/*
3737
* rxrpc defined keys take an arbitrary string as the description and an
@@ -1042,12 +1042,12 @@ EXPORT_SYMBOL(rxrpc_get_null_key);
10421042
* - this returns the result in XDR form
10431043
*/
10441044
static long rxrpc_read(const struct key *key,
1045-
char __user *buffer, size_t buflen)
1045+
char *buffer, size_t buflen)
10461046
{
10471047
const struct rxrpc_key_token *token;
10481048
const struct krb5_principal *princ;
10491049
size_t size;
1050-
__be32 __user *xdr, *oldxdr;
1050+
__be32 *xdr, *oldxdr;
10511051
u32 cnlen, toksize, ntoks, tok, zero;
10521052
u16 toksizes[AFSTOKEN_MAX];
10531053
int loop;
@@ -1124,30 +1124,25 @@ static long rxrpc_read(const struct key *key,
11241124
if (!buffer || buflen < size)
11251125
return size;
11261126

1127-
xdr = (__be32 __user *) buffer;
1127+
xdr = (__be32 *)buffer;
11281128
zero = 0;
11291129
#define ENCODE(x) \
11301130
do { \
1131-
__be32 y = htonl(x); \
1132-
if (put_user(y, xdr++) < 0) \
1133-
goto fault; \
1131+
*xdr++ = htonl(x); \
11341132
} while(0)
11351133
#define ENCODE_DATA(l, s) \
11361134
do { \
11371135
u32 _l = (l); \
11381136
ENCODE(l); \
1139-
if (copy_to_user(xdr, (s), _l) != 0) \
1140-
goto fault; \
1141-
if (_l & 3 && \
1142-
copy_to_user((u8 __user *)xdr + _l, &zero, 4 - (_l & 3)) != 0) \
1143-
goto fault; \
1137+
memcpy(xdr, (s), _l); \
1138+
if (_l & 3) \
1139+
memcpy((u8 *)xdr + _l, &zero, 4 - (_l & 3)); \
11441140
xdr += (_l + 3) >> 2; \
11451141
} while(0)
11461142
#define ENCODE64(x) \
11471143
do { \
11481144
__be64 y = cpu_to_be64(x); \
1149-
if (copy_to_user(xdr, &y, 8) != 0) \
1150-
goto fault; \
1145+
memcpy(xdr, &y, 8); \
11511146
xdr += 8 >> 2; \
11521147
} while(0)
11531148
#define ENCODE_STR(s) \
@@ -1238,8 +1233,4 @@ static long rxrpc_read(const struct key *key,
12381233
ASSERTCMP((char __user *) xdr - buffer, ==, size);
12391234
_leave(" = %zu", size);
12401235
return size;
1241-
1242-
fault:
1243-
_leave(" = -EFAULT");
1244-
return -EFAULT;
12451236
}

‎security/keys/big_key.c‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
352352
* read the key data
353353
* - the key's semaphore is read-locked
354354
*/
355-
long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
355+
long big_key_read(const struct key *key, char *buffer, size_t buflen)
356356
{
357357
size_t datalen = (size_t)key->payload.data[big_key_len];
358358
long ret;
@@ -391,19 +391,16 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
391391

392392
ret = datalen;
393393

394-
/* copy decrypted data to user */
395-
if (copy_to_user(buffer, buf->virt, datalen) != 0)
396-
ret = -EFAULT;
394+
/* copy out decrypted data */
395+
memcpy(buffer, buf->virt, datalen);
397396

398397
err_fput:
399398
fput(file);
400399
error:
401400
big_key_free_buffer(buf);
402401
} else {
403402
ret = datalen;
404-
if (copy_to_user(buffer, key->payload.data[big_key_data],
405-
datalen) != 0)
406-
ret = -EFAULT;
403+
memcpy(buffer, key->payload.data[big_key_data], datalen);
407404
}
408405

409406
return ret;

‎security/keys/encrypted-keys/encrypted.c‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
902902
}
903903

904904
/*
905-
* encrypted_read - format and copy the encrypted data to userspace
905+
* encrypted_read - format and copy out the encrypted data
906906
*
907907
* The resulting datablob format is:
908908
* <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
909909
*
910910
* On success, return to userspace the encrypted key datablob size.
911911
*/
912-
static long encrypted_read(const struct key *key, char __user *buffer,
912+
static long encrypted_read(const struct key *key, char *buffer,
913913
size_t buflen)
914914
{
915915
struct encrypted_key_payload *epayload;
@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
957957
key_put(mkey);
958958
memzero_explicit(derived_key, sizeof(derived_key));
959959

960-
if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
961-
ret = -EFAULT;
960+
memcpy(buffer, ascii_buf, asciiblob_len);
962961
kzfree(ascii_buf);
963962

964963
return asciiblob_len;

‎security/keys/keyctl.c‎

Lines changed: 57 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -797,6 +797,21 @@ long keyctl_keyring_search(key_serial_t ringid,
797797
return ret;
798798
}
799799

800+
/*
801+
* Call the read method
802+
*/
803+
static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
804+
{
805+
long ret;
806+
807+
down_read(&key->sem);
808+
ret = key_validate(key);
809+
if (ret == 0)
810+
ret = key->type->read(key, buffer, buflen);
811+
up_read(&key->sem);
812+
return ret;
813+
}
814+
800815
/*
801816
* Read a key's payload.
802817
*
@@ -812,53 +827,79 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
812827
struct key *key;
813828
key_ref_t key_ref;
814829
long ret;
830+
char *key_data;
815831

816832
/* find the key first */
817833
key_ref = lookup_user_key(keyid, 0, 0);
818834
if (IS_ERR(key_ref)) {
819835
ret = -ENOKEY;
820-
goto error;
836+
goto out;
821837
}
822838

823839
key = key_ref_to_ptr(key_ref);
824840

825841
ret = key_read_state(key);
826842
if (ret < 0)
827-
goto error2; /* Negatively instantiated */
843+
goto key_put_out; /* Negatively instantiated */
828844

829845
/* see if we can read it directly */
830846
ret = key_permission(key_ref, KEY_NEED_READ);
831847
if (ret == 0)
832848
goto can_read_key;
833849
if (ret != -EACCES)
834-
goto error2;
850+
goto key_put_out;
835851

836852
/* we can't; see if it's searchable from this process's keyrings
837853
* - we automatically take account of the fact that it may be
838854
* dangling off an instantiation key
839855
*/
840856
if (!is_key_possessed(key_ref)) {
841857
ret = -EACCES;
842-
goto error2;
858+
goto key_put_out;
843859
}
844860

845861
/* the key is probably readable - now try to read it */
846862
can_read_key:
847-
ret = -EOPNOTSUPP;
848-
if (key->type->read) {
849-
/* Read the data with the semaphore held (since we might sleep)
850-
* to protect against the key being updated or revoked.
851-
*/
852-
down_read(&key->sem);
853-
ret = key_validate(key);
854-
if (ret == 0)
855-
ret = key->type->read(key, buffer, buflen);
856-
up_read(&key->sem);
863+
if (!key->type->read) {
864+
ret = -EOPNOTSUPP;
865+
goto key_put_out;
857866
}
858867

859-
error2:
868+
if (!buffer || !buflen) {
869+
/* Get the key length from the read method */
870+
ret = __keyctl_read_key(key, NULL, 0);
871+
goto key_put_out;
872+
}
873+
874+
/*
875+
* Read the data with the semaphore held (since we might sleep)
876+
* to protect against the key being updated or revoked.
877+
*
878+
* Allocating a temporary buffer to hold the keys before
879+
* transferring them to user buffer to avoid potential
880+
* deadlock involving page fault and mmap_sem.
881+
*/
882+
key_data = kmalloc(buflen, GFP_KERNEL);
883+
884+
if (!key_data) {
885+
ret = -ENOMEM;
886+
goto key_put_out;
887+
}
888+
ret = __keyctl_read_key(key, key_data, buflen);
889+
890+
/*
891+
* Read methods will just return the required length without
892+
* any copying if the provided length isn't large enough.
893+
*/
894+
if (ret > 0 && ret <= buflen) {
895+
if (copy_to_user(buffer, key_data, ret))
896+
ret = -EFAULT;
897+
}
898+
kzfree(key_data);
899+
900+
key_put_out:
860901
key_put(key);
861-
error:
902+
out:
862903
return ret;
863904
}
864905

‎security/keys/keyring.c‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -459,18 +459,14 @@ static int keyring_read_iterator(const void *object, void *data)
459459
{
460460
struct keyring_read_iterator_context *ctx = data;
461461
const struct key *key = keyring_ptr_to_key(object);
462-
int ret;
463462

464463
kenter("{%s,%d},,{%zu/%zu}",
465464
key->type->name, key->serial, ctx->count, ctx->buflen);
466465

467466
if (ctx->count >= ctx->buflen)
468467
return 1;
469468

470-
ret = put_user(key->serial, ctx->buffer);
471-
if (ret < 0)
472-
return ret;
473-
ctx->buffer++;
469+
*ctx->buffer++ = key->serial;
474470
ctx->count += sizeof(key->serial);
475471
return 0;
476472
}

‎security/keys/request_key_auth.c‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
2222
static void request_key_auth_describe(const struct key *, struct seq_file *);
2323
static void request_key_auth_revoke(struct key *);
2424
static void request_key_auth_destroy(struct key *);
25-
static long request_key_auth_read(const struct key *, char __user *, size_t);
25+
static long request_key_auth_read(const struct key *, char *, size_t);
2626

2727
/*
2828
* The request-key authorisation key type definition.
@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
8080
* - the key's semaphore is read-locked
8181
*/
8282
static long request_key_auth_read(const struct key *key,
83-
char __user *buffer, size_t buflen)
83+
char *buffer, size_t buflen)
8484
{
8585
struct request_key_auth *rka = dereference_key_locked(key);
8686
size_t datalen;
@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
9797
if (buflen > datalen)
9898
buflen = datalen;
9999

100-
if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
101-
ret = -EFAULT;
100+
memcpy(buffer, rka->callout_info, buflen);
102101
}
103102

104103
return ret;

0 commit comments

Comments
 (0)