|
| 1 | +keys: Fix linking a duplicate key to a keyring's assoc_array |
| 2 | + |
| 3 | +jira LE-1907 |
| 4 | +Rebuild_History Non-Buildable kernel-4.18.0-553.5.1.el8_10 |
| 5 | +commit-author Petr Pavlu < [email protected]> |
| 6 | +commit d55901522f96082a43b9842d34867363c0cdbac5 |
| 7 | +Empty-Commit: Cherry-Pick Conflicts during history rebuild. |
| 8 | +Will be included in final tarball splat. Ref for failed cherry-pick at: |
| 9 | +ciq/ciq_backports/kernel-4.18.0-553.5.1.el8_10/d5590152.failed |
| 10 | + |
| 11 | +When making a DNS query inside the kernel using dns_query(), the request |
| 12 | +code can in rare cases end up creating a duplicate index key in the |
| 13 | +assoc_array of the destination keyring. It is eventually found by |
| 14 | +a BUG_ON() check in the assoc_array implementation and results in |
| 15 | +a crash. |
| 16 | + |
| 17 | +Example report: |
| 18 | +[2158499.700025] kernel BUG at ../lib/assoc_array.c:652! |
| 19 | +[2158499.700039] invalid opcode: 0000 [#1] SMP PTI |
| 20 | +[2158499.700065] CPU: 3 PID: 31985 Comm: kworker/3:1 Kdump: loaded Not tainted 5.3.18-150300.59.90-default #1 SLE15-SP3 |
| 21 | +[2158499.700096] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020 |
| 22 | +[2158499.700351] Workqueue: cifsiod cifs_resolve_server [cifs] |
| 23 | +[2158499.700380] RIP: 0010:assoc_array_insert+0x85f/0xa40 |
| 24 | +[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 |
| 25 | +[2158499.700448] RSP: 0018:ffffc0bd6187faf0 EFLAGS: 00010282 |
| 26 | +[2158499.700470] RAX: ffff9f1ea7da2fe8 RBX: ffff9f1ea7da2fc1 RCX: 0000000000000005 |
| 27 | +[2158499.700492] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000000 |
| 28 | +[2158499.700515] RBP: ffffc0bd6187fbb0 R08: ffff9f185faf1100 R09: 0000000000000000 |
| 29 | +[2158499.700538] R10: ffff9f1ea7da2cc0 R11: 000000005ed8cec8 R12: ffffc0bd6187fc28 |
| 30 | +[2158499.700561] R13: ffff9f15feb8d000 R14: ffff9f1ea7da2fc0 R15: ffff9f168dc0d740 |
| 31 | +[2158499.700585] FS: 0000000000000000(0000) GS:ffff9f185fac0000(0000) knlGS:0000000000000000 |
| 32 | +[2158499.700610] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 |
| 33 | +[2158499.700630] CR2: 00007fdd94fca238 CR3: 0000000809d8c006 CR4: 00000000003706e0 |
| 34 | +[2158499.700702] Call Trace: |
| 35 | +[2158499.700741] ? key_alloc+0x447/0x4b0 |
| 36 | +[2158499.700768] ? __key_link_begin+0x43/0xa0 |
| 37 | +[2158499.700790] __key_link_begin+0x43/0xa0 |
| 38 | +[2158499.700814] request_key_and_link+0x2c7/0x730 |
| 39 | +[2158499.700847] ? dns_resolver_read+0x20/0x20 [dns_resolver] |
| 40 | +[2158499.700873] ? key_default_cmp+0x20/0x20 |
| 41 | +[2158499.700898] request_key_tag+0x43/0xa0 |
| 42 | +[2158499.700926] dns_query+0x114/0x2ca [dns_resolver] |
| 43 | +[2158499.701127] dns_resolve_server_name_to_ip+0x194/0x310 [cifs] |
| 44 | +[2158499.701164] ? scnprintf+0x49/0x90 |
| 45 | +[2158499.701190] ? __switch_to_asm+0x40/0x70 |
| 46 | +[2158499.701211] ? __switch_to_asm+0x34/0x70 |
| 47 | +[2158499.701405] reconn_set_ipaddr_from_hostname+0x81/0x2a0 [cifs] |
| 48 | +[2158499.701603] cifs_resolve_server+0x4b/0xd0 [cifs] |
| 49 | +[2158499.701632] process_one_work+0x1f8/0x3e0 |
| 50 | +[2158499.701658] worker_thread+0x2d/0x3f0 |
| 51 | +[2158499.701682] ? process_one_work+0x3e0/0x3e0 |
| 52 | +[2158499.701703] kthread+0x10d/0x130 |
| 53 | +[2158499.701723] ? kthread_park+0xb0/0xb0 |
| 54 | +[2158499.701746] ret_from_fork+0x1f/0x40 |
| 55 | + |
| 56 | +The situation occurs as follows: |
| 57 | +* Some kernel facility invokes dns_query() to resolve a hostname, for |
| 58 | + example, "abcdef". The function registers its global DNS resolver |
| 59 | + cache as current->cred.thread_keyring and passes the query to |
| 60 | + request_key_net() -> request_key_tag() -> request_key_and_link(). |
| 61 | +* Function request_key_and_link() creates a keyring_search_context |
| 62 | + object. Its match_data.cmp method gets set via a call to |
| 63 | + type->match_preparse() (resolves to dns_resolver_match_preparse()) to |
| 64 | + dns_resolver_cmp(). |
| 65 | +* Function request_key_and_link() continues and invokes |
| 66 | + search_process_keyrings_rcu() which returns that a given key was not |
| 67 | + found. The control is then passed to request_key_and_link() -> |
| 68 | + construct_alloc_key(). |
| 69 | +* Concurrently to that, a second task similarly makes a DNS query for |
| 70 | + "abcdef." and its result gets inserted into the DNS resolver cache. |
| 71 | +* Back on the first task, function construct_alloc_key() first runs |
| 72 | + __key_link_begin() to determine an assoc_array_edit operation to |
| 73 | + insert a new key. Index keys in the array are compared exactly as-is, |
| 74 | + using keyring_compare_object(). The operation finds that "abcdef" is |
| 75 | + not yet present in the destination keyring. |
| 76 | +* Function construct_alloc_key() continues and checks if a given key is |
| 77 | + already present on some keyring by again calling |
| 78 | + search_process_keyrings_rcu(). This search is done using |
| 79 | + dns_resolver_cmp() and "abcdef" gets matched with now present key |
| 80 | + "abcdef.". |
| 81 | +* The found key is linked on the destination keyring by calling |
| 82 | + __key_link() and using the previously calculated assoc_array_edit |
| 83 | + operation. This inserts the "abcdef." key in the array but creates |
| 84 | + a duplicity because the same index key is already present. |
| 85 | + |
| 86 | +Fix the problem by postponing __key_link_begin() in |
| 87 | +construct_alloc_key() until an actual key which should be linked into |
| 88 | +the destination keyring is determined. |
| 89 | + |
| 90 | +[ [email protected]: added a fixes tag and cc to stable] |
| 91 | + |
| 92 | +Fixes: df593ee23e05 ("keys: Hoist locking out of __key_link_begin()") |
| 93 | + Signed-off-by: Petr Pavlu < [email protected]> |
| 94 | + Reviewed-by: Joey Lee < [email protected]> |
| 95 | + Reviewed-by: Jarkko Sakkinen < [email protected]> |
| 96 | + Signed-off-by: Jarkko Sakkinen < [email protected]> |
| 97 | +(cherry picked from commit d55901522f96082a43b9842d34867363c0cdbac5) |
| 98 | + Signed-off-by: Jonathan Maple < [email protected]> |
| 99 | + |
| 100 | +# Conflicts: |
| 101 | +# security/keys/request_key.c |
| 102 | +diff --cc security/keys/request_key.c |
| 103 | +index 41a00ff10085,a7673ad86d18..000000000000 |
| 104 | +--- a/security/keys/request_key.c |
| 105 | ++++ b/security/keys/request_key.c |
| 106 | +@@@ -386,25 -401,35 +386,38 @@@ static int construct_alloc_key(struct k |
| 107 | + set_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags); |
| 108 | + |
| 109 | + if (dest_keyring) { |
| 110 | +- ret = __key_link_lock(dest_keyring, &ctx->index_key); |
| 111 | ++ ret = __key_link_lock(dest_keyring, &key->index_key); |
| 112 | + if (ret < 0) |
| 113 | + goto link_lock_failed; |
| 114 | +- ret = __key_link_begin(dest_keyring, &ctx->index_key, &edit); |
| 115 | +- if (ret < 0) |
| 116 | +- goto link_prealloc_failed; |
| 117 | + } |
| 118 | + |
| 119 | +- /* attach the key to the destination keyring under lock, but we do need |
| 120 | ++ /* |
| 121 | ++ * Attach the key to the destination keyring under lock, but we do need |
| 122 | + * to do another check just in case someone beat us to it whilst we |
| 123 | +- * waited for locks */ |
| 124 | ++ * waited for locks. |
| 125 | ++ * |
| 126 | ++ * The caller might specify a comparison function which looks for keys |
| 127 | ++ * that do not exactly match but are still equivalent from the caller's |
| 128 | ++ * perspective. The __key_link_begin() operation must be done only after |
| 129 | ++ * an actual key is determined. |
| 130 | ++ */ |
| 131 | + mutex_lock(&key_construction_mutex); |
| 132 | + |
| 133 | + - rcu_read_lock(); |
| 134 | + - key_ref = search_process_keyrings_rcu(ctx); |
| 135 | + - rcu_read_unlock(); |
| 136 | + + key_ref = search_process_keyrings(ctx); |
| 137 | + if (!IS_ERR(key_ref)) |
| 138 | + goto key_already_present; |
| 139 | + |
| 140 | +++<<<<<<< HEAD |
| 141 | + + if (dest_keyring) |
| 142 | + + __key_link(key, &edit); |
| 143 | +++======= |
| 144 | ++ if (dest_keyring) { |
| 145 | ++ ret = __key_link_begin(dest_keyring, &key->index_key, &edit); |
| 146 | ++ if (ret < 0) |
| 147 | ++ goto link_alloc_failed; |
| 148 | ++ __key_link(dest_keyring, key, &edit); |
| 149 | ++ } |
| 150 | +++>>>>>>> d55901522f96 (keys: Fix linking a duplicate key to a keyring's assoc_array) |
| 151 | + |
| 152 | + mutex_unlock(&key_construction_mutex); |
| 153 | + if (dest_keyring) |
| 154 | +@@@ -421,10 -446,13 +434,18 @@@ key_already_present |
| 155 | + mutex_unlock(&key_construction_mutex); |
| 156 | + key = key_ref_to_ptr(key_ref); |
| 157 | + if (dest_keyring) { |
| 158 | ++ ret = __key_link_begin(dest_keyring, &key->index_key, &edit); |
| 159 | ++ if (ret < 0) |
| 160 | ++ goto link_alloc_failed_unlocked; |
| 161 | + ret = __key_link_check_live_key(dest_keyring, key); |
| 162 | + if (ret == 0) |
| 163 | +++<<<<<<< HEAD |
| 164 | + + __key_link(key, &edit); |
| 165 | + + __key_link_end(dest_keyring, &ctx->index_key, edit); |
| 166 | +++======= |
| 167 | ++ __key_link(dest_keyring, key, &edit); |
| 168 | ++ __key_link_end(dest_keyring, &key->index_key, edit); |
| 169 | +++>>>>>>> d55901522f96 (keys: Fix linking a duplicate key to a keyring's assoc_array) |
| 170 | + if (ret < 0) |
| 171 | + goto link_check_failed; |
| 172 | + } |
| 173 | +* Unmerged path security/keys/request_key.c |
0 commit comments