diff mbox series

keys: Fix linking a duplicate key to a keyring's assoc_array

Message ID 20230323130412.32097-1-petr.pavlu@suse.com
State New
Headers
Series keys: Fix linking a duplicate key to a keyring's assoc_array |

Commit Message

Petr Pavlu March 23, 2023, 1:04 p.m. UTC 
  When making a DNS query inside the kernel using dns_query(), the request
code can in rare cases end up creating a duplicate index key in the
assoc_array of the destination keyring. It is eventually found by
a BUG_ON() check in the assoc_array implementation and results in
a crash.

Example report:
[2158499.700025] kernel BUG at ../lib/assoc_array.c:652!
[2158499.700039] invalid opcode: 0000 [#1] SMP PTI
[2158499.700065] CPU: 3 PID: 31985 Comm: kworker/3:1 Kdump: loaded Not tainted 5.3.18-150300.59.90-default #1 SLE15-SP3
[2158499.700096] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
[2158499.700351] Workqueue: cifsiod cifs_resolve_server [cifs]
[2158499.700380] RIP: 0010:assoc_array_insert+0x85f/0xa40
[2158499.700401] Code: ff 74 2b 48 8b 3b 49 8b 45 18 4c 89 e6 48 83 e7 fe e8 95 ec 74 00 3b 45 88 7d db 85 c0 79 d4 0f 0b 0f 0b 0f 0b e8 41 f2 be ff <0f> 0b 0f 0b 81 7d 88 ff ff ff 7f 4c 89 eb 4c 8b ad 58 ff ff ff 0f
[2158499.700448] RSP: 0018:ffffc0bd6187faf0 EFLAGS: 00010282
[2158499.700470] RAX: ffff9f1ea7da2fe8 RBX: ffff9f1ea7da2fc1 RCX: 0000000000000005
[2158499.700492] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000000
[2158499.700515] RBP: ffffc0bd6187fbb0 R08: ffff9f185faf1100 R09: 0000000000000000
[2158499.700538] R10: ffff9f1ea7da2cc0 R11: 000000005ed8cec8 R12: ffffc0bd6187fc28
[2158499.700561] R13: ffff9f15feb8d000 R14: ffff9f1ea7da2fc0 R15: ffff9f168dc0d740
[2158499.700585] FS:  0000000000000000(0000) GS:ffff9f185fac0000(0000) knlGS:0000000000000000
[2158499.700610] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[2158499.700630] CR2: 00007fdd94fca238 CR3: 0000000809d8c006 CR4: 00000000003706e0
[2158499.700702] Call Trace:
[2158499.700741]  ? key_alloc+0x447/0x4b0
[2158499.700768]  ? __key_link_begin+0x43/0xa0
[2158499.700790]  __key_link_begin+0x43/0xa0
[2158499.700814]  request_key_and_link+0x2c7/0x730
[2158499.700847]  ? dns_resolver_read+0x20/0x20 [dns_resolver]
[2158499.700873]  ? key_default_cmp+0x20/0x20
[2158499.700898]  request_key_tag+0x43/0xa0
[2158499.700926]  dns_query+0x114/0x2ca [dns_resolver]
[2158499.701127]  dns_resolve_server_name_to_ip+0x194/0x310 [cifs]
[2158499.701164]  ? scnprintf+0x49/0x90
[2158499.701190]  ? __switch_to_asm+0x40/0x70
[2158499.701211]  ? __switch_to_asm+0x34/0x70
[2158499.701405]  reconn_set_ipaddr_from_hostname+0x81/0x2a0 [cifs]
[2158499.701603]  cifs_resolve_server+0x4b/0xd0 [cifs]
[2158499.701632]  process_one_work+0x1f8/0x3e0
[2158499.701658]  worker_thread+0x2d/0x3f0
[2158499.701682]  ? process_one_work+0x3e0/0x3e0
[2158499.701703]  kthread+0x10d/0x130
[2158499.701723]  ? kthread_park+0xb0/0xb0
[2158499.701746]  ret_from_fork+0x1f/0x40

The situation occurs as follows:
* Some kernel facility invokes dns_query() to resolve a hostname, for
  example, "abcdef". The function registers its global DNS resolver
  cache as current->cred.thread_keyring and passes the query to
  request_key_net() -> request_key_tag() -> request_key_and_link().
* Function request_key_and_link() creates a keyring_search_context
  object. Its match_data.cmp method gets set via a call to
  type->match_preparse() (resolves to dns_resolver_match_preparse()) to
  dns_resolver_cmp().
* Function request_key_and_link() continues and invokes
  search_process_keyrings_rcu() which returns that a given key was not
  found. The control is then passed to request_key_and_link() ->
  construct_alloc_key().
* Concurrently to that, a second task similarly makes a DNS query for
  "abcdef." and its result gets inserted into the DNS resolver cache.
* Back on the first task, function construct_alloc_key() first runs
  __key_link_begin() to determine an assoc_array_edit operation to
  insert a new key. Index keys in the array are compared exactly as-is,
  using keyring_compare_object(). The operation finds that "abcdef" is
  not yet present in the destination keyring.
* Function construct_alloc_key() continues and checks if a given key is
  already present on some keyring by again calling
  search_process_keyrings_rcu(). This search is done using
  dns_resolver_cmp() and "abcdef" gets matched with now present key
  "abcdef.".
* The found key is linked on the destination keyring by calling
  __key_link() and using the previously calculated assoc_array_edit
  operation. This inserts the "abcdef." key in the array but creates
  a duplicity because the same index key is already present.

Fix the problem by postponing __key_link_begin() in
construct_alloc_key() until an actual key which should be linked into
the destination keyring is determined.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 security/keys/request_key.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Comments

Jarkko Sakkinen March 30, 2023, 12:13 a.m. UTC | #1 
On Thu, Mar 23, 2023 at 02:04:12PM +0100, Petr Pavlu wrote:
> When making a DNS query inside the kernel using dns_query(), the request
> code can in rare cases end up creating a duplicate index key in the
> assoc_array of the destination keyring. It is eventually found by
> a BUG_ON() check in the assoc_array implementation and results in
> a crash.
> 
> Example report:
> [2158499.700025] kernel BUG at ../lib/assoc_array.c:652!
> [2158499.700039] invalid opcode: 0000 [#1] SMP PTI
> [2158499.700065] CPU: 3 PID: 31985 Comm: kworker/3:1 Kdump: loaded Not tainted 5.3.18-150300.59.90-default #1 SLE15-SP3
> [2158499.700096] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
> [2158499.700351] Workqueue: cifsiod cifs_resolve_server [cifs]
> [2158499.700380] RIP: 0010:assoc_array_insert+0x85f/0xa40
> [2158499.700401] Code: ff 74 2b 48 8b 3b 49 8b 45 18 4c 89 e6 48 83 e7 fe e8 95 ec 74 00 3b 45 88 7d db 85 c0 79 d4 0f 0b 0f 0b 0f 0b e8 41 f2 be ff <0f> 0b 0f 0b 81 7d 88 ff ff ff 7f 4c 89 eb 4c 8b ad 58 ff ff ff 0f
> [2158499.700448] RSP: 0018:ffffc0bd6187faf0 EFLAGS: 00010282
> [2158499.700470] RAX: ffff9f1ea7da2fe8 RBX: ffff9f1ea7da2fc1 RCX: 0000000000000005
> [2158499.700492] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000000
> [2158499.700515] RBP: ffffc0bd6187fbb0 R08: ffff9f185faf1100 R09: 0000000000000000
> [2158499.700538] R10: ffff9f1ea7da2cc0 R11: 000000005ed8cec8 R12: ffffc0bd6187fc28
> [2158499.700561] R13: ffff9f15feb8d000 R14: ffff9f1ea7da2fc0 R15: ffff9f168dc0d740
> [2158499.700585] FS:  0000000000000000(0000) GS:ffff9f185fac0000(0000) knlGS:0000000000000000
> [2158499.700610] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [2158499.700630] CR2: 00007fdd94fca238 CR3: 0000000809d8c006 CR4: 00000000003706e0
> [2158499.700702] Call Trace:
> [2158499.700741]  ? key_alloc+0x447/0x4b0
> [2158499.700768]  ? __key_link_begin+0x43/0xa0
> [2158499.700790]  __key_link_begin+0x43/0xa0
> [2158499.700814]  request_key_and_link+0x2c7/0x730
> [2158499.700847]  ? dns_resolver_read+0x20/0x20 [dns_resolver]
> [2158499.700873]  ? key_default_cmp+0x20/0x20
> [2158499.700898]  request_key_tag+0x43/0xa0
> [2158499.700926]  dns_query+0x114/0x2ca [dns_resolver]
> [2158499.701127]  dns_resolve_server_name_to_ip+0x194/0x310 [cifs]
> [2158499.701164]  ? scnprintf+0x49/0x90
> [2158499.701190]  ? __switch_to_asm+0x40/0x70
> [2158499.701211]  ? __switch_to_asm+0x34/0x70
> [2158499.701405]  reconn_set_ipaddr_from_hostname+0x81/0x2a0 [cifs]
> [2158499.701603]  cifs_resolve_server+0x4b/0xd0 [cifs]
> [2158499.701632]  process_one_work+0x1f8/0x3e0
> [2158499.701658]  worker_thread+0x2d/0x3f0
> [2158499.701682]  ? process_one_work+0x3e0/0x3e0
> [2158499.701703]  kthread+0x10d/0x130
> [2158499.701723]  ? kthread_park+0xb0/0xb0
> [2158499.701746]  ret_from_fork+0x1f/0x40
> 
> The situation occurs as follows:
> * Some kernel facility invokes dns_query() to resolve a hostname, for
>   example, "abcdef". The function registers its global DNS resolver
>   cache as current->cred.thread_keyring and passes the query to
>   request_key_net() -> request_key_tag() -> request_key_and_link().
> * Function request_key_and_link() creates a keyring_search_context
>   object. Its match_data.cmp method gets set via a call to
>   type->match_preparse() (resolves to dns_resolver_match_preparse()) to
>   dns_resolver_cmp().
> * Function request_key_and_link() continues and invokes
>   search_process_keyrings_rcu() which returns that a given key was not
>   found. The control is then passed to request_key_and_link() ->
>   construct_alloc_key().
> * Concurrently to that, a second task similarly makes a DNS query for
>   "abcdef." and its result gets inserted into the DNS resolver cache.
> * Back on the first task, function construct_alloc_key() first runs
>   __key_link_begin() to determine an assoc_array_edit operation to
>   insert a new key. Index keys in the array are compared exactly as-is,
>   using keyring_compare_object(). The operation finds that "abcdef" is
>   not yet present in the destination keyring.
> * Function construct_alloc_key() continues and checks if a given key is
>   already present on some keyring by again calling
>   search_process_keyrings_rcu(). This search is done using
>   dns_resolver_cmp() and "abcdef" gets matched with now present key
>   "abcdef.".
> * The found key is linked on the destination keyring by calling
>   __key_link() and using the previously calculated assoc_array_edit
>   operation. This inserts the "abcdef." key in the array but creates
>   a duplicity because the same index key is already present.
> 
> Fix the problem by postponing __key_link_begin() in
> construct_alloc_key() until an actual key which should be linked into
> the destination keyring is determined.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  security/keys/request_key.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 2da4404276f0..04eb7e4cedad 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -398,17 +398,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
>  
>  	if (dest_keyring) {
> -		ret = __key_link_lock(dest_keyring, &ctx->index_key);
> +		ret = __key_link_lock(dest_keyring, &key->index_key);
>  		if (ret < 0)
>  			goto link_lock_failed;
> -		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
> -		if (ret < 0)
> -			goto link_prealloc_failed;
>  	}
>  
> -	/* attach the key to the destination keyring under lock, but we do need
> +	/*
> +	 * Attach the key to the destination keyring under lock, but we do need
>  	 * to do another check just in case someone beat us to it whilst we
> -	 * waited for locks */
> +	 * waited for locks.
> +	 *
> +	 * The caller might specify a comparison function which looks for keys
> +	 * that do not exactly match but are still equivalent from the caller's
> +	 * perspective. The __key_link_begin() operation must be done only after
> +	 * an actual key is determined.
> +	 */
>  	mutex_lock(&key_construction_mutex);
>  
>  	rcu_read_lock();
> @@ -417,12 +421,16 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	if (!IS_ERR(key_ref))
>  		goto key_already_present;
>  
> -	if (dest_keyring)
> +	if (dest_keyring) {
> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> +		if (ret < 0)
> +			goto link_alloc_failed;
>  		__key_link(dest_keyring, key, &edit);
> +	}
>  
>  	mutex_unlock(&key_construction_mutex);
>  	if (dest_keyring)
> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> +		__key_link_end(dest_keyring, &key->index_key, edit);
>  	mutex_unlock(&user->cons_lock);
>  	*_key = key;
>  	kleave(" = 0 [%d]", key_serial(key));
> @@ -435,10 +443,13 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	mutex_unlock(&key_construction_mutex);
>  	key = key_ref_to_ptr(key_ref);
>  	if (dest_keyring) {
> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> +		if (ret < 0)
> +			goto link_alloc_failed_unlocked;
>  		ret = __key_link_check_live_key(dest_keyring, key);
>  		if (ret == 0)
>  			__key_link(dest_keyring, key, &edit);
> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> +		__key_link_end(dest_keyring, &key->index_key, edit);
>  		if (ret < 0)
>  			goto link_check_failed;
>  	}
> @@ -453,8 +464,10 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	kleave(" = %d [linkcheck]", ret);
>  	return ret;
>  
> -link_prealloc_failed:
> -	__key_link_end(dest_keyring, &ctx->index_key, edit);
> +link_alloc_failed:
> +	mutex_unlock(&key_construction_mutex);
> +link_alloc_failed_unlocked:
> +	__key_link_end(dest_keyring, &key->index_key, edit);
>  link_lock_failed:
>  	mutex_unlock(&user->cons_lock);
>  	key_put(key);
> -- 
> 2.35.3
> 

A good catch, thanks.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko
Lee, Chun-Yi April 21, 2023, 2:39 a.m. UTC | #2 
On Thu, Mar 23, 2023 at 02:04:12PM +0100, Petr Pavlu wrote:
> When making a DNS query inside the kernel using dns_query(), the request
> code can in rare cases end up creating a duplicate index key in the
> assoc_array of the destination keyring. It is eventually found by
> a BUG_ON() check in the assoc_array implementation and results in
> a crash.
> 
> Example report:
> [2158499.700025] kernel BUG at ../lib/assoc_array.c:652!
> [2158499.700039] invalid opcode: 0000 [#1] SMP PTI
> [2158499.700065] CPU: 3 PID: 31985 Comm: kworker/3:1 Kdump: loaded Not tainted 5.3.18-150300.59.90-default #1 SLE15-SP3
> [2158499.700096] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
> [2158499.700351] Workqueue: cifsiod cifs_resolve_server [cifs]
> [2158499.700380] RIP: 0010:assoc_array_insert+0x85f/0xa40
> [2158499.700401] Code: ff 74 2b 48 8b 3b 49 8b 45 18 4c 89 e6 48 83 e7 fe e8 95 ec 74 00 3b 45 88 7d db 85 c0 79 d4 0f 0b 0f 0b 0f 0b e8 41 f2 be ff <0f> 0b 0f 0b 81 7d 88 ff ff ff 7f 4c 89 eb 4c 8b ad 58 ff ff ff 0f
> [2158499.700448] RSP: 0018:ffffc0bd6187faf0 EFLAGS: 00010282
> [2158499.700470] RAX: ffff9f1ea7da2fe8 RBX: ffff9f1ea7da2fc1 RCX: 0000000000000005
> [2158499.700492] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000000
> [2158499.700515] RBP: ffffc0bd6187fbb0 R08: ffff9f185faf1100 R09: 0000000000000000
> [2158499.700538] R10: ffff9f1ea7da2cc0 R11: 000000005ed8cec8 R12: ffffc0bd6187fc28
> [2158499.700561] R13: ffff9f15feb8d000 R14: ffff9f1ea7da2fc0 R15: ffff9f168dc0d740
> [2158499.700585] FS:  0000000000000000(0000) GS:ffff9f185fac0000(0000) knlGS:0000000000000000
> [2158499.700610] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [2158499.700630] CR2: 00007fdd94fca238 CR3: 0000000809d8c006 CR4: 00000000003706e0
> [2158499.700702] Call Trace:
> [2158499.700741]  ? key_alloc+0x447/0x4b0
> [2158499.700768]  ? __key_link_begin+0x43/0xa0
> [2158499.700790]  __key_link_begin+0x43/0xa0
> [2158499.700814]  request_key_and_link+0x2c7/0x730
> [2158499.700847]  ? dns_resolver_read+0x20/0x20 [dns_resolver]
> [2158499.700873]  ? key_default_cmp+0x20/0x20
> [2158499.700898]  request_key_tag+0x43/0xa0
> [2158499.700926]  dns_query+0x114/0x2ca [dns_resolver]
> [2158499.701127]  dns_resolve_server_name_to_ip+0x194/0x310 [cifs]
> [2158499.701164]  ? scnprintf+0x49/0x90
> [2158499.701190]  ? __switch_to_asm+0x40/0x70
> [2158499.701211]  ? __switch_to_asm+0x34/0x70
> [2158499.701405]  reconn_set_ipaddr_from_hostname+0x81/0x2a0 [cifs]
> [2158499.701603]  cifs_resolve_server+0x4b/0xd0 [cifs]
> [2158499.701632]  process_one_work+0x1f8/0x3e0
> [2158499.701658]  worker_thread+0x2d/0x3f0
> [2158499.701682]  ? process_one_work+0x3e0/0x3e0
> [2158499.701703]  kthread+0x10d/0x130
> [2158499.701723]  ? kthread_park+0xb0/0xb0
> [2158499.701746]  ret_from_fork+0x1f/0x40
> 
> The situation occurs as follows:
> * Some kernel facility invokes dns_query() to resolve a hostname, for
>   example, "abcdef". The function registers its global DNS resolver
>   cache as current->cred.thread_keyring and passes the query to
>   request_key_net() -> request_key_tag() -> request_key_and_link().
> * Function request_key_and_link() creates a keyring_search_context
>   object. Its match_data.cmp method gets set via a call to
>   type->match_preparse() (resolves to dns_resolver_match_preparse()) to
>   dns_resolver_cmp().
> * Function request_key_and_link() continues and invokes
>   search_process_keyrings_rcu() which returns that a given key was not
>   found. The control is then passed to request_key_and_link() ->
>   construct_alloc_key().
> * Concurrently to that, a second task similarly makes a DNS query for
>   "abcdef." and its result gets inserted into the DNS resolver cache.
> * Back on the first task, function construct_alloc_key() first runs
>   __key_link_begin() to determine an assoc_array_edit operation to
>   insert a new key. Index keys in the array are compared exactly as-is,
>   using keyring_compare_object(). The operation finds that "abcdef" is
>   not yet present in the destination keyring.
> * Function construct_alloc_key() continues and checks if a given key is
>   already present on some keyring by again calling
>   search_process_keyrings_rcu(). This search is done using
>   dns_resolver_cmp() and "abcdef" gets matched with now present key
>   "abcdef.".
> * The found key is linked on the destination keyring by calling
>   __key_link() and using the previously calculated assoc_array_edit
>   operation. This inserts the "abcdef." key in the array but creates
>   a duplicity because the same index key is already present.
> 
> Fix the problem by postponing __key_link_begin() in
> construct_alloc_key() until an actual key which should be linked into
> the destination keyring is determined.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>

I have reviewed this patch. Feel free to add: 

Reviewed-by: Joey Lee <jlee@suse.com>

Thanks
Joey Lee

> ---
>  security/keys/request_key.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 2da4404276f0..04eb7e4cedad 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -398,17 +398,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
>  
>  	if (dest_keyring) {
> -		ret = __key_link_lock(dest_keyring, &ctx->index_key);
> +		ret = __key_link_lock(dest_keyring, &key->index_key);
>  		if (ret < 0)
>  			goto link_lock_failed;
> -		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
> -		if (ret < 0)
> -			goto link_prealloc_failed;
>  	}
>  
> -	/* attach the key to the destination keyring under lock, but we do need
> +	/*
> +	 * Attach the key to the destination keyring under lock, but we do need
>  	 * to do another check just in case someone beat us to it whilst we
> -	 * waited for locks */
> +	 * waited for locks.
> +	 *
> +	 * The caller might specify a comparison function which looks for keys
> +	 * that do not exactly match but are still equivalent from the caller's
> +	 * perspective. The __key_link_begin() operation must be done only after
> +	 * an actual key is determined.
> +	 */
>  	mutex_lock(&key_construction_mutex);
>  
>  	rcu_read_lock();
> @@ -417,12 +421,16 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	if (!IS_ERR(key_ref))
>  		goto key_already_present;
>  
> -	if (dest_keyring)
> +	if (dest_keyring) {
> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> +		if (ret < 0)
> +			goto link_alloc_failed;
>  		__key_link(dest_keyring, key, &edit);
> +	}
>  
>  	mutex_unlock(&key_construction_mutex);
>  	if (dest_keyring)
> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> +		__key_link_end(dest_keyring, &key->index_key, edit);
>  	mutex_unlock(&user->cons_lock);
>  	*_key = key;
>  	kleave(" = 0 [%d]", key_serial(key));
> @@ -435,10 +443,13 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	mutex_unlock(&key_construction_mutex);
>  	key = key_ref_to_ptr(key_ref);
>  	if (dest_keyring) {
> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> +		if (ret < 0)
> +			goto link_alloc_failed_unlocked;
>  		ret = __key_link_check_live_key(dest_keyring, key);
>  		if (ret == 0)
>  			__key_link(dest_keyring, key, &edit);
> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> +		__key_link_end(dest_keyring, &key->index_key, edit);
>  		if (ret < 0)
>  			goto link_check_failed;
>  	}
> @@ -453,8 +464,10 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	kleave(" = %d [linkcheck]", ret);
>  	return ret;
>  
> -link_prealloc_failed:
> -	__key_link_end(dest_keyring, &ctx->index_key, edit);
> +link_alloc_failed:
> +	mutex_unlock(&key_construction_mutex);
> +link_alloc_failed_unlocked:
> +	__key_link_end(dest_keyring, &key->index_key, edit);
>  link_lock_failed:
>  	mutex_unlock(&user->cons_lock);
>  	key_put(key);
Petr Pavlu June 8, 2023, 9:55 a.m. UTC | #3 
On 3/30/23 02:13, Jarkko Sakkinen wrote:
> On Thu, Mar 23, 2023 at 02:04:12PM +0100, Petr Pavlu wrote:
>> When making a DNS query inside the kernel using dns_query(), the request
>> code can in rare cases end up creating a duplicate index key in the
>> assoc_array of the destination keyring. It is eventually found by
>> a BUG_ON() check in the assoc_array implementation and results in
>> a crash.
>>
>> Example report:
>> [2158499.700025] kernel BUG at ../lib/assoc_array.c:652!
>> [2158499.700039] invalid opcode: 0000 [#1] SMP PTI
>> [2158499.700065] CPU: 3 PID: 31985 Comm: kworker/3:1 Kdump: loaded Not tainted 5.3.18-150300.59.90-default #1 SLE15-SP3
>> [2158499.700096] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
>> [2158499.700351] Workqueue: cifsiod cifs_resolve_server [cifs]
>> [2158499.700380] RIP: 0010:assoc_array_insert+0x85f/0xa40
>> [2158499.700401] Code: ff 74 2b 48 8b 3b 49 8b 45 18 4c 89 e6 48 83 e7 fe e8 95 ec 74 00 3b 45 88 7d db 85 c0 79 d4 0f 0b 0f 0b 0f 0b e8 41 f2 be ff <0f> 0b 0f 0b 81 7d 88 ff ff ff 7f 4c 89 eb 4c 8b ad 58 ff ff ff 0f
>> [2158499.700448] RSP: 0018:ffffc0bd6187faf0 EFLAGS: 00010282
>> [2158499.700470] RAX: ffff9f1ea7da2fe8 RBX: ffff9f1ea7da2fc1 RCX: 0000000000000005
>> [2158499.700492] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000000
>> [2158499.700515] RBP: ffffc0bd6187fbb0 R08: ffff9f185faf1100 R09: 0000000000000000
>> [2158499.700538] R10: ffff9f1ea7da2cc0 R11: 000000005ed8cec8 R12: ffffc0bd6187fc28
>> [2158499.700561] R13: ffff9f15feb8d000 R14: ffff9f1ea7da2fc0 R15: ffff9f168dc0d740
>> [2158499.700585] FS:  0000000000000000(0000) GS:ffff9f185fac0000(0000) knlGS:0000000000000000
>> [2158499.700610] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [2158499.700630] CR2: 00007fdd94fca238 CR3: 0000000809d8c006 CR4: 00000000003706e0
>> [2158499.700702] Call Trace:
>> [2158499.700741]  ? key_alloc+0x447/0x4b0
>> [2158499.700768]  ? __key_link_begin+0x43/0xa0
>> [2158499.700790]  __key_link_begin+0x43/0xa0
>> [2158499.700814]  request_key_and_link+0x2c7/0x730
>> [2158499.700847]  ? dns_resolver_read+0x20/0x20 [dns_resolver]
>> [2158499.700873]  ? key_default_cmp+0x20/0x20
>> [2158499.700898]  request_key_tag+0x43/0xa0
>> [2158499.700926]  dns_query+0x114/0x2ca [dns_resolver]
>> [2158499.701127]  dns_resolve_server_name_to_ip+0x194/0x310 [cifs]
>> [2158499.701164]  ? scnprintf+0x49/0x90
>> [2158499.701190]  ? __switch_to_asm+0x40/0x70
>> [2158499.701211]  ? __switch_to_asm+0x34/0x70
>> [2158499.701405]  reconn_set_ipaddr_from_hostname+0x81/0x2a0 [cifs]
>> [2158499.701603]  cifs_resolve_server+0x4b/0xd0 [cifs]
>> [2158499.701632]  process_one_work+0x1f8/0x3e0
>> [2158499.701658]  worker_thread+0x2d/0x3f0
>> [2158499.701682]  ? process_one_work+0x3e0/0x3e0
>> [2158499.701703]  kthread+0x10d/0x130
>> [2158499.701723]  ? kthread_park+0xb0/0xb0
>> [2158499.701746]  ret_from_fork+0x1f/0x40
>>
>> The situation occurs as follows:
>> * Some kernel facility invokes dns_query() to resolve a hostname, for
>>   example, "abcdef". The function registers its global DNS resolver
>>   cache as current->cred.thread_keyring and passes the query to
>>   request_key_net() -> request_key_tag() -> request_key_and_link().
>> * Function request_key_and_link() creates a keyring_search_context
>>   object. Its match_data.cmp method gets set via a call to
>>   type->match_preparse() (resolves to dns_resolver_match_preparse()) to
>>   dns_resolver_cmp().
>> * Function request_key_and_link() continues and invokes
>>   search_process_keyrings_rcu() which returns that a given key was not
>>   found. The control is then passed to request_key_and_link() ->
>>   construct_alloc_key().
>> * Concurrently to that, a second task similarly makes a DNS query for
>>   "abcdef." and its result gets inserted into the DNS resolver cache.
>> * Back on the first task, function construct_alloc_key() first runs
>>   __key_link_begin() to determine an assoc_array_edit operation to
>>   insert a new key. Index keys in the array are compared exactly as-is,
>>   using keyring_compare_object(). The operation finds that "abcdef" is
>>   not yet present in the destination keyring.
>> * Function construct_alloc_key() continues and checks if a given key is
>>   already present on some keyring by again calling
>>   search_process_keyrings_rcu(). This search is done using
>>   dns_resolver_cmp() and "abcdef" gets matched with now present key
>>   "abcdef.".
>> * The found key is linked on the destination keyring by calling
>>   __key_link() and using the previously calculated assoc_array_edit
>>   operation. This inserts the "abcdef." key in the array but creates
>>   a duplicity because the same index key is already present.
>>
>> Fix the problem by postponing __key_link_begin() in
>> construct_alloc_key() until an actual key which should be linked into
>> the destination keyring is determined.
>>
>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>> ---
>>  security/keys/request_key.c | 35 ++++++++++++++++++++++++-----------
>>  1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
>> index 2da4404276f0..04eb7e4cedad 100644
>> --- a/security/keys/request_key.c
>> +++ b/security/keys/request_key.c
>> @@ -398,17 +398,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>>  	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
>>  
>>  	if (dest_keyring) {
>> -		ret = __key_link_lock(dest_keyring, &ctx->index_key);
>> +		ret = __key_link_lock(dest_keyring, &key->index_key);
>>  		if (ret < 0)
>>  			goto link_lock_failed;
>> -		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
>> -		if (ret < 0)
>> -			goto link_prealloc_failed;
>>  	}
>>  
>> -	/* attach the key to the destination keyring under lock, but we do need
>> +	/*
>> +	 * Attach the key to the destination keyring under lock, but we do need
>>  	 * to do another check just in case someone beat us to it whilst we
>> -	 * waited for locks */
>> +	 * waited for locks.
>> +	 *
>> +	 * The caller might specify a comparison function which looks for keys
>> +	 * that do not exactly match but are still equivalent from the caller's
>> +	 * perspective. The __key_link_begin() operation must be done only after
>> +	 * an actual key is determined.
>> +	 */
>>  	mutex_lock(&key_construction_mutex);
>>  
>>  	rcu_read_lock();
>> @@ -417,12 +421,16 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>>  	if (!IS_ERR(key_ref))
>>  		goto key_already_present;
>>  
>> -	if (dest_keyring)
>> +	if (dest_keyring) {
>> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
>> +		if (ret < 0)
>> +			goto link_alloc_failed;
>>  		__key_link(dest_keyring, key, &edit);
>> +	}
>>  
>>  	mutex_unlock(&key_construction_mutex);
>>  	if (dest_keyring)
>> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
>> +		__key_link_end(dest_keyring, &key->index_key, edit);
>>  	mutex_unlock(&user->cons_lock);
>>  	*_key = key;
>>  	kleave(" = 0 [%d]", key_serial(key));
>> @@ -435,10 +443,13 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>>  	mutex_unlock(&key_construction_mutex);
>>  	key = key_ref_to_ptr(key_ref);
>>  	if (dest_keyring) {
>> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
>> +		if (ret < 0)
>> +			goto link_alloc_failed_unlocked;
>>  		ret = __key_link_check_live_key(dest_keyring, key);
>>  		if (ret == 0)
>>  			__key_link(dest_keyring, key, &edit);
>> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
>> +		__key_link_end(dest_keyring, &key->index_key, edit);
>>  		if (ret < 0)
>>  			goto link_check_failed;
>>  	}
>> @@ -453,8 +464,10 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>>  	kleave(" = %d [linkcheck]", ret);
>>  	return ret;
>>  
>> -link_prealloc_failed:
>> -	__key_link_end(dest_keyring, &ctx->index_key, edit);
>> +link_alloc_failed:
>> +	mutex_unlock(&key_construction_mutex);
>> +link_alloc_failed_unlocked:
>> +	__key_link_end(dest_keyring, &key->index_key, edit);
>>  link_lock_failed:
>>  	mutex_unlock(&user->cons_lock);
>>  	key_put(key);
>> -- 
>> 2.35.3
>>
> 
> A good catch, thanks.
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Thank you for the review. Can this be picked through your tree?

Cheers,
Petr
Jarkko Sakkinen June 8, 2023, 1:18 p.m. UTC | #4 
On Thu Jun 8, 2023 at 12:55 PM EEST, Petr Pavlu wrote:
> On 3/30/23 02:13, Jarkko Sakkinen wrote:
> > On Thu, Mar 23, 2023 at 02:04:12PM +0100, Petr Pavlu wrote:
> >> When making a DNS query inside the kernel using dns_query(), the request
> >> code can in rare cases end up creating a duplicate index key in the
> >> assoc_array of the destination keyring. It is eventually found by
> >> a BUG_ON() check in the assoc_array implementation and results in
> >> a crash.
> >>
> >> Example report:
> >> [2158499.700025] kernel BUG at ../lib/assoc_array.c:652!
> >> [2158499.700039] invalid opcode: 0000 [#1] SMP PTI
> >> [2158499.700065] CPU: 3 PID: 31985 Comm: kworker/3:1 Kdump: loaded Not tainted 5.3.18-150300.59.90-default #1 SLE15-SP3
> >> [2158499.700096] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
> >> [2158499.700351] Workqueue: cifsiod cifs_resolve_server [cifs]
> >> [2158499.700380] RIP: 0010:assoc_array_insert+0x85f/0xa40
> >> [2158499.700401] Code: ff 74 2b 48 8b 3b 49 8b 45 18 4c 89 e6 48 83 e7 fe e8 95 ec 74 00 3b 45 88 7d db 85 c0 79 d4 0f 0b 0f 0b 0f 0b e8 41 f2 be ff <0f> 0b 0f 0b 81 7d 88 ff ff ff 7f 4c 89 eb 4c 8b ad 58 ff ff ff 0f
> >> [2158499.700448] RSP: 0018:ffffc0bd6187faf0 EFLAGS: 00010282
> >> [2158499.700470] RAX: ffff9f1ea7da2fe8 RBX: ffff9f1ea7da2fc1 RCX: 0000000000000005
> >> [2158499.700492] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000000
> >> [2158499.700515] RBP: ffffc0bd6187fbb0 R08: ffff9f185faf1100 R09: 0000000000000000
> >> [2158499.700538] R10: ffff9f1ea7da2cc0 R11: 000000005ed8cec8 R12: ffffc0bd6187fc28
> >> [2158499.700561] R13: ffff9f15feb8d000 R14: ffff9f1ea7da2fc0 R15: ffff9f168dc0d740
> >> [2158499.700585] FS:  0000000000000000(0000) GS:ffff9f185fac0000(0000) knlGS:0000000000000000
> >> [2158499.700610] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [2158499.700630] CR2: 00007fdd94fca238 CR3: 0000000809d8c006 CR4: 00000000003706e0
> >> [2158499.700702] Call Trace:
> >> [2158499.700741]  ? key_alloc+0x447/0x4b0
> >> [2158499.700768]  ? __key_link_begin+0x43/0xa0
> >> [2158499.700790]  __key_link_begin+0x43/0xa0
> >> [2158499.700814]  request_key_and_link+0x2c7/0x730
> >> [2158499.700847]  ? dns_resolver_read+0x20/0x20 [dns_resolver]
> >> [2158499.700873]  ? key_default_cmp+0x20/0x20
> >> [2158499.700898]  request_key_tag+0x43/0xa0
> >> [2158499.700926]  dns_query+0x114/0x2ca [dns_resolver]
> >> [2158499.701127]  dns_resolve_server_name_to_ip+0x194/0x310 [cifs]
> >> [2158499.701164]  ? scnprintf+0x49/0x90
> >> [2158499.701190]  ? __switch_to_asm+0x40/0x70
> >> [2158499.701211]  ? __switch_to_asm+0x34/0x70
> >> [2158499.701405]  reconn_set_ipaddr_from_hostname+0x81/0x2a0 [cifs]
> >> [2158499.701603]  cifs_resolve_server+0x4b/0xd0 [cifs]
> >> [2158499.701632]  process_one_work+0x1f8/0x3e0
> >> [2158499.701658]  worker_thread+0x2d/0x3f0
> >> [2158499.701682]  ? process_one_work+0x3e0/0x3e0
> >> [2158499.701703]  kthread+0x10d/0x130
> >> [2158499.701723]  ? kthread_park+0xb0/0xb0
> >> [2158499.701746]  ret_from_fork+0x1f/0x40
> >>
> >> The situation occurs as follows:
> >> * Some kernel facility invokes dns_query() to resolve a hostname, for
> >>   example, "abcdef". The function registers its global DNS resolver
> >>   cache as current->cred.thread_keyring and passes the query to
> >>   request_key_net() -> request_key_tag() -> request_key_and_link().
> >> * Function request_key_and_link() creates a keyring_search_context
> >>   object. Its match_data.cmp method gets set via a call to
> >>   type->match_preparse() (resolves to dns_resolver_match_preparse()) to
> >>   dns_resolver_cmp().
> >> * Function request_key_and_link() continues and invokes
> >>   search_process_keyrings_rcu() which returns that a given key was not
> >>   found. The control is then passed to request_key_and_link() ->
> >>   construct_alloc_key().
> >> * Concurrently to that, a second task similarly makes a DNS query for
> >>   "abcdef." and its result gets inserted into the DNS resolver cache.
> >> * Back on the first task, function construct_alloc_key() first runs
> >>   __key_link_begin() to determine an assoc_array_edit operation to
> >>   insert a new key. Index keys in the array are compared exactly as-is,
> >>   using keyring_compare_object(). The operation finds that "abcdef" is
> >>   not yet present in the destination keyring.
> >> * Function construct_alloc_key() continues and checks if a given key is
> >>   already present on some keyring by again calling
> >>   search_process_keyrings_rcu(). This search is done using
> >>   dns_resolver_cmp() and "abcdef" gets matched with now present key
> >>   "abcdef.".
> >> * The found key is linked on the destination keyring by calling
> >>   __key_link() and using the previously calculated assoc_array_edit
> >>   operation. This inserts the "abcdef." key in the array but creates
> >>   a duplicity because the same index key is already present.
> >>
> >> Fix the problem by postponing __key_link_begin() in
> >> construct_alloc_key() until an actual key which should be linked into
> >> the destination keyring is determined.
> >>
> >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> >> ---
> >>  security/keys/request_key.c | 35 ++++++++++++++++++++++++-----------
> >>  1 file changed, 24 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> >> index 2da4404276f0..04eb7e4cedad 100644
> >> --- a/security/keys/request_key.c
> >> +++ b/security/keys/request_key.c
> >> @@ -398,17 +398,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> >>  	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
> >>  
> >>  	if (dest_keyring) {
> >> -		ret = __key_link_lock(dest_keyring, &ctx->index_key);
> >> +		ret = __key_link_lock(dest_keyring, &key->index_key);
> >>  		if (ret < 0)
> >>  			goto link_lock_failed;
> >> -		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
> >> -		if (ret < 0)
> >> -			goto link_prealloc_failed;
> >>  	}
> >>  
> >> -	/* attach the key to the destination keyring under lock, but we do need
> >> +	/*
> >> +	 * Attach the key to the destination keyring under lock, but we do need
> >>  	 * to do another check just in case someone beat us to it whilst we
> >> -	 * waited for locks */
> >> +	 * waited for locks.
> >> +	 *
> >> +	 * The caller might specify a comparison function which looks for keys
> >> +	 * that do not exactly match but are still equivalent from the caller's
> >> +	 * perspective. The __key_link_begin() operation must be done only after
> >> +	 * an actual key is determined.
> >> +	 */
> >>  	mutex_lock(&key_construction_mutex);
> >>  
> >>  	rcu_read_lock();
> >> @@ -417,12 +421,16 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> >>  	if (!IS_ERR(key_ref))
> >>  		goto key_already_present;
> >>  
> >> -	if (dest_keyring)
> >> +	if (dest_keyring) {
> >> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> >> +		if (ret < 0)
> >> +			goto link_alloc_failed;
> >>  		__key_link(dest_keyring, key, &edit);
> >> +	}
> >>  
> >>  	mutex_unlock(&key_construction_mutex);
> >>  	if (dest_keyring)
> >> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> >> +		__key_link_end(dest_keyring, &key->index_key, edit);
> >>  	mutex_unlock(&user->cons_lock);
> >>  	*_key = key;
> >>  	kleave(" = 0 [%d]", key_serial(key));
> >> @@ -435,10 +443,13 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> >>  	mutex_unlock(&key_construction_mutex);
> >>  	key = key_ref_to_ptr(key_ref);
> >>  	if (dest_keyring) {
> >> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> >> +		if (ret < 0)
> >> +			goto link_alloc_failed_unlocked;
> >>  		ret = __key_link_check_live_key(dest_keyring, key);
> >>  		if (ret == 0)
> >>  			__key_link(dest_keyring, key, &edit);
> >> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> >> +		__key_link_end(dest_keyring, &key->index_key, edit);
> >>  		if (ret < 0)
> >>  			goto link_check_failed;
> >>  	}
> >> @@ -453,8 +464,10 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> >>  	kleave(" = %d [linkcheck]", ret);
> >>  	return ret;
> >>  
> >> -link_prealloc_failed:
> >> -	__key_link_end(dest_keyring, &ctx->index_key, edit);
> >> +link_alloc_failed:
> >> +	mutex_unlock(&key_construction_mutex);
> >> +link_alloc_failed_unlocked:
> >> +	__key_link_end(dest_keyring, &key->index_key, edit);
> >>  link_lock_failed:
> >>  	mutex_unlock(&user->cons_lock);
> >>  	key_put(key);
> >> -- 
> >> 2.35.3
> >>
> > 
> > A good catch, thanks.
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> Thank you for the review. Can this be picked through your tree?
>
> Cheers,
> Petr

Hi, I pressed send too early in my respose. I was going to say that
I'm picking.

I did recently from mutt to aerc, and sometimes get really confused
what is going on :-)

BR, Jarkko
Jarkko Sakkinen June 8, 2023, 1:28 p.m. UTC | #5 
On Thu Jun 8, 2023 at 4:18 PM EEST, Jarkko Sakkinen wrote:
> On Thu Jun 8, 2023 at 12:55 PM EEST, Petr Pavlu wrote:
> > On 3/30/23 02:13, Jarkko Sakkinen wrote:
> > > On Thu, Mar 23, 2023 at 02:04:12PM +0100, Petr Pavlu wrote:
> > >> When making a DNS query inside the kernel using dns_query(), the request
> > >> code can in rare cases end up creating a duplicate index key in the
> > >> assoc_array of the destination keyring. It is eventually found by
> > >> a BUG_ON() check in the assoc_array implementation and results in
> > >> a crash.
> > >>
> > >> Example report:
> > >> [2158499.700025] kernel BUG at ../lib/assoc_array.c:652!
> > >> [2158499.700039] invalid opcode: 0000 [#1] SMP PTI
> > >> [2158499.700065] CPU: 3 PID: 31985 Comm: kworker/3:1 Kdump: loaded Not tainted 5.3.18-150300.59.90-default #1 SLE15-SP3
> > >> [2158499.700096] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
> > >> [2158499.700351] Workqueue: cifsiod cifs_resolve_server [cifs]
> > >> [2158499.700380] RIP: 0010:assoc_array_insert+0x85f/0xa40
> > >> [2158499.700401] Code: ff 74 2b 48 8b 3b 49 8b 45 18 4c 89 e6 48 83 e7 fe e8 95 ec 74 00 3b 45 88 7d db 85 c0 79 d4 0f 0b 0f 0b 0f 0b e8 41 f2 be ff <0f> 0b 0f 0b 81 7d 88 ff ff ff 7f 4c 89 eb 4c 8b ad 58 ff ff ff 0f
> > >> [2158499.700448] RSP: 0018:ffffc0bd6187faf0 EFLAGS: 00010282
> > >> [2158499.700470] RAX: ffff9f1ea7da2fe8 RBX: ffff9f1ea7da2fc1 RCX: 0000000000000005
> > >> [2158499.700492] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000000
> > >> [2158499.700515] RBP: ffffc0bd6187fbb0 R08: ffff9f185faf1100 R09: 0000000000000000
> > >> [2158499.700538] R10: ffff9f1ea7da2cc0 R11: 000000005ed8cec8 R12: ffffc0bd6187fc28
> > >> [2158499.700561] R13: ffff9f15feb8d000 R14: ffff9f1ea7da2fc0 R15: ffff9f168dc0d740
> > >> [2158499.700585] FS:  0000000000000000(0000) GS:ffff9f185fac0000(0000) knlGS:0000000000000000
> > >> [2158499.700610] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> [2158499.700630] CR2: 00007fdd94fca238 CR3: 0000000809d8c006 CR4: 00000000003706e0
> > >> [2158499.700702] Call Trace:
> > >> [2158499.700741]  ? key_alloc+0x447/0x4b0
> > >> [2158499.700768]  ? __key_link_begin+0x43/0xa0
> > >> [2158499.700790]  __key_link_begin+0x43/0xa0
> > >> [2158499.700814]  request_key_and_link+0x2c7/0x730
> > >> [2158499.700847]  ? dns_resolver_read+0x20/0x20 [dns_resolver]
> > >> [2158499.700873]  ? key_default_cmp+0x20/0x20
> > >> [2158499.700898]  request_key_tag+0x43/0xa0
> > >> [2158499.700926]  dns_query+0x114/0x2ca [dns_resolver]
> > >> [2158499.701127]  dns_resolve_server_name_to_ip+0x194/0x310 [cifs]
> > >> [2158499.701164]  ? scnprintf+0x49/0x90
> > >> [2158499.701190]  ? __switch_to_asm+0x40/0x70
> > >> [2158499.701211]  ? __switch_to_asm+0x34/0x70
> > >> [2158499.701405]  reconn_set_ipaddr_from_hostname+0x81/0x2a0 [cifs]
> > >> [2158499.701603]  cifs_resolve_server+0x4b/0xd0 [cifs]
> > >> [2158499.701632]  process_one_work+0x1f8/0x3e0
> > >> [2158499.701658]  worker_thread+0x2d/0x3f0
> > >> [2158499.701682]  ? process_one_work+0x3e0/0x3e0
> > >> [2158499.701703]  kthread+0x10d/0x130
> > >> [2158499.701723]  ? kthread_park+0xb0/0xb0
> > >> [2158499.701746]  ret_from_fork+0x1f/0x40
> > >>
> > >> The situation occurs as follows:
> > >> * Some kernel facility invokes dns_query() to resolve a hostname, for
> > >>   example, "abcdef". The function registers its global DNS resolver
> > >>   cache as current->cred.thread_keyring and passes the query to
> > >>   request_key_net() -> request_key_tag() -> request_key_and_link().
> > >> * Function request_key_and_link() creates a keyring_search_context
> > >>   object. Its match_data.cmp method gets set via a call to
> > >>   type->match_preparse() (resolves to dns_resolver_match_preparse()) to
> > >>   dns_resolver_cmp().
> > >> * Function request_key_and_link() continues and invokes
> > >>   search_process_keyrings_rcu() which returns that a given key was not
> > >>   found. The control is then passed to request_key_and_link() ->
> > >>   construct_alloc_key().
> > >> * Concurrently to that, a second task similarly makes a DNS query for
> > >>   "abcdef." and its result gets inserted into the DNS resolver cache.
> > >> * Back on the first task, function construct_alloc_key() first runs
> > >>   __key_link_begin() to determine an assoc_array_edit operation to
> > >>   insert a new key. Index keys in the array are compared exactly as-is,
> > >>   using keyring_compare_object(). The operation finds that "abcdef" is
> > >>   not yet present in the destination keyring.
> > >> * Function construct_alloc_key() continues and checks if a given key is
> > >>   already present on some keyring by again calling
> > >>   search_process_keyrings_rcu(). This search is done using
> > >>   dns_resolver_cmp() and "abcdef" gets matched with now present key
> > >>   "abcdef.".
> > >> * The found key is linked on the destination keyring by calling
> > >>   __key_link() and using the previously calculated assoc_array_edit
> > >>   operation. This inserts the "abcdef." key in the array but creates
> > >>   a duplicity because the same index key is already present.
> > >>
> > >> Fix the problem by postponing __key_link_begin() in
> > >> construct_alloc_key() until an actual key which should be linked into
> > >> the destination keyring is determined.
> > >>
> > >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> > >> ---
> > >>  security/keys/request_key.c | 35 ++++++++++++++++++++++++-----------
> > >>  1 file changed, 24 insertions(+), 11 deletions(-)
> > >>
> > >> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> > >> index 2da4404276f0..04eb7e4cedad 100644
> > >> --- a/security/keys/request_key.c
> > >> +++ b/security/keys/request_key.c
> > >> @@ -398,17 +398,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> > >>  	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
> > >>  
> > >>  	if (dest_keyring) {
> > >> -		ret = __key_link_lock(dest_keyring, &ctx->index_key);
> > >> +		ret = __key_link_lock(dest_keyring, &key->index_key);
> > >>  		if (ret < 0)
> > >>  			goto link_lock_failed;
> > >> -		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
> > >> -		if (ret < 0)
> > >> -			goto link_prealloc_failed;
> > >>  	}
> > >>  
> > >> -	/* attach the key to the destination keyring under lock, but we do need
> > >> +	/*
> > >> +	 * Attach the key to the destination keyring under lock, but we do need
> > >>  	 * to do another check just in case someone beat us to it whilst we
> > >> -	 * waited for locks */
> > >> +	 * waited for locks.
> > >> +	 *
> > >> +	 * The caller might specify a comparison function which looks for keys
> > >> +	 * that do not exactly match but are still equivalent from the caller's
> > >> +	 * perspective. The __key_link_begin() operation must be done only after
> > >> +	 * an actual key is determined.
> > >> +	 */
> > >>  	mutex_lock(&key_construction_mutex);
> > >>  
> > >>  	rcu_read_lock();
> > >> @@ -417,12 +421,16 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> > >>  	if (!IS_ERR(key_ref))
> > >>  		goto key_already_present;
> > >>  
> > >> -	if (dest_keyring)
> > >> +	if (dest_keyring) {
> > >> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> > >> +		if (ret < 0)
> > >> +			goto link_alloc_failed;
> > >>  		__key_link(dest_keyring, key, &edit);
> > >> +	}
> > >>  
> > >>  	mutex_unlock(&key_construction_mutex);
> > >>  	if (dest_keyring)
> > >> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> > >> +		__key_link_end(dest_keyring, &key->index_key, edit);
> > >>  	mutex_unlock(&user->cons_lock);
> > >>  	*_key = key;
> > >>  	kleave(" = 0 [%d]", key_serial(key));
> > >> @@ -435,10 +443,13 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> > >>  	mutex_unlock(&key_construction_mutex);
> > >>  	key = key_ref_to_ptr(key_ref);
> > >>  	if (dest_keyring) {
> > >> +		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> > >> +		if (ret < 0)
> > >> +			goto link_alloc_failed_unlocked;
> > >>  		ret = __key_link_check_live_key(dest_keyring, key);
> > >>  		if (ret == 0)
> > >>  			__key_link(dest_keyring, key, &edit);
> > >> -		__key_link_end(dest_keyring, &ctx->index_key, edit);
> > >> +		__key_link_end(dest_keyring, &key->index_key, edit);
> > >>  		if (ret < 0)
> > >>  			goto link_check_failed;
> > >>  	}
> > >> @@ -453,8 +464,10 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
> > >>  	kleave(" = %d [linkcheck]", ret);
> > >>  	return ret;
> > >>  
> > >> -link_prealloc_failed:
> > >> -	__key_link_end(dest_keyring, &ctx->index_key, edit);
> > >> +link_alloc_failed:
> > >> +	mutex_unlock(&key_construction_mutex);
> > >> +link_alloc_failed_unlocked:
> > >> +	__key_link_end(dest_keyring, &key->index_key, edit);
> > >>  link_lock_failed:
> > >>  	mutex_unlock(&user->cons_lock);
> > >>  	key_put(key);
> > >> -- 
> > >> 2.35.3
> > >>
> > > 
> > > A good catch, thanks.
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > Thank you for the review. Can this be picked through your tree?
> >
> > Cheers,
> > Petr
>
> Hi, I pressed send too early in my respose. I was going to say that
> I'm picking.
>
> I did recently from mutt to aerc, and sometimes get really confused
> what is going on :-)

OK, now it is applied:

https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=8ea234bb14b53f3bf1ce63dd669d4acbc519ab6d

BR, Jarkko
David Howells June 8, 2023, 2:12 p.m. UTC | #6 
Apologies for missing this patch.

Petr Pavlu <petr.pavlu@suse.com> wrote:

> * Back on the first task, function construct_alloc_key() first runs
>   __key_link_begin() to determine an assoc_array_edit operation to
>   insert a new key. Index keys in the array are compared exactly as-is,
>   using keyring_compare_object(). The operation finds that "abcdef" is
>   not yet present in the destination keyring.

Good catch, but I think it's probably the wrong solution.

keyring_compare_object() needs to use the ->cmp() function from the key type.

It's not just request_key() that might have a problem, but also key_link().

There are also asymmetric keys which match against multiple criteria - though
I'm fine with just matching the main description there (the important bit
being to maintain the integrity of the tree inside assoc_array).

I wonder, should I scrap the assoc_array approach and go to each keyring being
a linked-list?  It's slower, but a lot easier to manage - and more forgiving
of problems.

	struct key_ptr {
		struct list_head	link;
		struct key		*key;
		unsigned long		key_hash;
	};

I'm also wondering if I should remove the key type from the matching criteria
- i.e. there can only be one key with any particular description in a ring at
once, regardless of type.  Unfortunately, this is may be a UAPI breaker
somewhere.

Any thoughts?

David
Jarkko Sakkinen June 8, 2023, 7:03 p.m. UTC | #7 
On Thu Jun 8, 2023 at 5:12 PM EEST, David Howells wrote:
> Apologies for missing this patch.
>
> Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> > * Back on the first task, function construct_alloc_key() first runs
> >   __key_link_begin() to determine an assoc_array_edit operation to
> >   insert a new key. Index keys in the array are compared exactly as-is,
> >   using keyring_compare_object(). The operation finds that "abcdef" is
> >   not yet present in the destination keyring.
>
> Good catch, but I think it's probably the wrong solution.
>
> keyring_compare_object() needs to use the ->cmp() function from the key type.
>
> It's not just request_key() that might have a problem, but also key_link().
>
> There are also asymmetric keys which match against multiple criteria - though
> I'm fine with just matching the main description there (the important bit
> being to maintain the integrity of the tree inside assoc_array).
>
> I wonder, should I scrap the assoc_array approach and go to each keyring being
> a linked-list?  It's slower, but a lot easier to manage - and more forgiving
> of problems.
>
> 	struct key_ptr {
> 		struct list_head	link;
> 		struct key		*key;
> 		unsigned long		key_hash;
> 	};
>
> I'm also wondering if I should remove the key type from the matching criteria
> - i.e. there can only be one key with any particular description in a ring at
> once, regardless of type.  Unfortunately, this is may be a UAPI breaker
> somewhere.
>
> Any thoughts?

If the amount of items stays at most in hundreds (or actually even like
few thousand items), there's very little gain of having the complexity
of associative array. In most cases it probably shoots back in many
ways.

I've been thinking this for a long time but have thought that since it
has been there, there must be good reasons to have it.

So yeah, definitely +1 for scraping assoc array.

BR, Jarkko
Petr Pavlu June 9, 2023, 9:49 a.m. UTC | #8 
On 6/8/23 16:12, David Howells wrote:
> Petr Pavlu <petr.pavlu@suse.com> wrote:
> 
>> * Back on the first task, function construct_alloc_key() first runs
>>   __key_link_begin() to determine an assoc_array_edit operation to
>>   insert a new key. Index keys in the array are compared exactly as-is,
>>   using keyring_compare_object(). The operation finds that "abcdef" is
>>   not yet present in the destination keyring.
> 
> Good catch, but I think it's probably the wrong solution.
> 
> keyring_compare_object() needs to use the ->cmp() function from the key type.
> 
> It's not just request_key() that might have a problem, but also key_link().

The way I view the current design is that it kind of consists of two
layers. Lower-level functions key_create(), key_link(), key_move(), etc.
are built directly on top of assoc_array, use the exact comparison and
benefit from the assoc_array speed.

Higher-level function request_key() then provides a callout
functionality and offers an option to do approximate search if a needed
key is already present. This gives a trade-off to potentially reduce
a number of callouts but on the other hand requires a linear search over
the underlying keyrings/assoc_arrays.

The patch tries to only provide a point fix where the request-key logic
in construct_alloc_key() wrongly interacted with the approximate
matching option. If my understanding of the current design is correct
then I think key_link() shouldn't require any change in this regard.

Just wanted to add this point, I can't really comment on whether the
whole thing should be designed differently in the first place.

Thanks,
Petr
diff mbox series

Patch

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 2da4404276f0..04eb7e4cedad 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -398,17 +398,21 @@  static int construct_alloc_key(struct keyring_search_context *ctx,
 	set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags);
 
 	if (dest_keyring) {
-		ret = __key_link_lock(dest_keyring, &ctx->index_key);
+		ret = __key_link_lock(dest_keyring, &key->index_key);
 		if (ret < 0)
 			goto link_lock_failed;
-		ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit);
-		if (ret < 0)
-			goto link_prealloc_failed;
 	}
 
-	/* attach the key to the destination keyring under lock, but we do need
+	/*
+	 * Attach the key to the destination keyring under lock, but we do need
 	 * to do another check just in case someone beat us to it whilst we
-	 * waited for locks */
+	 * waited for locks.
+	 *
+	 * The caller might specify a comparison function which looks for keys
+	 * that do not exactly match but are still equivalent from the caller's
+	 * perspective. The __key_link_begin() operation must be done only after
+	 * an actual key is determined.
+	 */
 	mutex_lock(&key_construction_mutex);
 
 	rcu_read_lock();
@@ -417,12 +421,16 @@  static int construct_alloc_key(struct keyring_search_context *ctx,
 	if (!IS_ERR(key_ref))
 		goto key_already_present;
 
-	if (dest_keyring)
+	if (dest_keyring) {
+		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
+		if (ret < 0)
+			goto link_alloc_failed;
 		__key_link(dest_keyring, key, &edit);
+	}
 
 	mutex_unlock(&key_construction_mutex);
 	if (dest_keyring)
-		__key_link_end(dest_keyring, &ctx->index_key, edit);
+		__key_link_end(dest_keyring, &key->index_key, edit);
 	mutex_unlock(&user->cons_lock);
 	*_key = key;
 	kleave(" = 0 [%d]", key_serial(key));
@@ -435,10 +443,13 @@  static int construct_alloc_key(struct keyring_search_context *ctx,
 	mutex_unlock(&key_construction_mutex);
 	key = key_ref_to_ptr(key_ref);
 	if (dest_keyring) {
+		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
+		if (ret < 0)
+			goto link_alloc_failed_unlocked;
 		ret = __key_link_check_live_key(dest_keyring, key);
 		if (ret == 0)
 			__key_link(dest_keyring, key, &edit);
-		__key_link_end(dest_keyring, &ctx->index_key, edit);
+		__key_link_end(dest_keyring, &key->index_key, edit);
 		if (ret < 0)
 			goto link_check_failed;
 	}
@@ -453,8 +464,10 @@  static int construct_alloc_key(struct keyring_search_context *ctx,
 	kleave(" = %d [linkcheck]", ret);
 	return ret;
 
-link_prealloc_failed:
-	__key_link_end(dest_keyring, &ctx->index_key, edit);
+link_alloc_failed:
+	mutex_unlock(&key_construction_mutex);
+link_alloc_failed_unlocked:
+	__key_link_end(dest_keyring, &key->index_key, edit);
 link_lock_failed:
 	mutex_unlock(&user->cons_lock);
 	key_put(key);