Commit 18779eac authored Mar 21, 2020 by Waiman Long Committed by Greg Kroah-Hartman Apr 23, 2020

KEYS: Don't write out to userspace while holding key semaphore



commit d3ec10aa95819bff18a0d936b18884c7816d0914 upstream.

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 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: ^1da177e4 ("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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

parent 9e303d52

include/keys/big_key-type.h

+1 −1

Original line number	Diff line number	Diff line
		@@ -21,6 +21,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 */

include/keys/user-type.h

+1 −2

Original line number	Diff line number	Diff line
		@@ -45,8 +45,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)
		{

include/linux/key-type.h

+1 −1

Original line number	Diff line number	Diff line
		@@ -125,7 +125,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)

net/dns_resolver/dns_key.c

+1 −1

Original line number	Diff line number	Diff line
		@@ -241,7 +241,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]);

net/rxrpc/key.c

+9 −18

Original line number	Diff line number	Diff line
		@@ -35,7 +35,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
		@@ -1044,12 +1044,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;
		@@ -1126,30 +1126,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) \
		@@ -1240,8 +1235,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;
		}