Browse code

Use dedicated multi->dco_peer_id for DCO instead of multi->peer_id

The lifetime and state machine of multi->peer_id does not exactly the
lifetime/state of DCO. This is especially for p2p NCP where a reconnection
can change the peer id. Also use this new field with value -1 to mean
not installed, replacing the dco_peer_added field.

Also ensure that we have a failure adding a new peer, we don't try to
set options for that peer or generating keys for it.

Patch v2: fix one comparison checking for 0 instead of -1
Patch v3: make recovery after failing dco_add_peer more robust
and the comparison that lead to not deleting a peer.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20221127090742.3487997-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/search?l=mid&q=20221127090742.3487997-1-arne@rfc2549.org
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Arne Schwabe authored on 2022/11/27 18:07:42
Showing 6 changed files
... ...
@@ -55,7 +55,7 @@ dco_install_key(struct tls_multi *multi, struct key_state *ks,
55 55
                 const char *ciphername)
56 56
 
57 57
 {
58
-    msg(D_DCO_DEBUG, "%s: peer_id=%d keyid=%d", __func__, multi->peer_id,
58
+    msg(D_DCO_DEBUG, "%s: peer_id=%d keyid=%d", __func__, multi->dco_peer_id,
59 59
         ks->key_id);
60 60
 
61 61
     /* Install a key in the PRIMARY slot only when no other key exist.
... ...
@@ -69,7 +69,7 @@ dco_install_key(struct tls_multi *multi, struct key_state *ks,
69 69
         slot = OVPN_KEY_SLOT_SECONDARY;
70 70
     }
71 71
 
72
-    int ret = dco_new_key(multi->dco, multi->peer_id, ks->key_id, slot,
72
+    int ret = dco_new_key(multi->dco, multi->dco_peer_id, ks->key_id, slot,
73 73
                           encrypt_key, encrypt_iv,
74 74
                           decrypt_key, decrypt_iv,
75 75
                           ciphername);
... ...
@@ -133,7 +133,7 @@ dco_get_secondary_key(struct tls_multi *multi, const struct key_state *primary)
133 133
 void
134 134
 dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
135 135
 {
136
-    msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id);
136
+    msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->dco_peer_id);
137 137
 
138 138
     /* this function checks if keys have to be swapped or erased, therefore it
139 139
      * can't do much if we don't have any key installed
... ...
@@ -151,14 +151,14 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
151 151
     {
152 152
         msg(D_DCO, "No encryption key found. Purging data channel keys");
153 153
 
154
-        int ret = dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY);
154
+        int ret = dco_del_key(dco, multi->dco_peer_id, OVPN_KEY_SLOT_PRIMARY);
155 155
         if (ret < 0)
156 156
         {
157 157
             msg(D_DCO, "Cannot delete primary key during wipe: %s (%d)", strerror(-ret), ret);
158 158
             return;
159 159
         }
160 160
 
161
-        ret = dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY);
161
+        ret = dco_del_key(dco, multi->dco_peer_id, OVPN_KEY_SLOT_SECONDARY);
162 162
         if (ret < 0)
163 163
         {
164 164
             msg(D_DCO, "Cannot delete secondary key during wipe: %s (%d)", strerror(-ret), ret);
... ...
@@ -184,7 +184,7 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
184 184
         msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d",
185 185
             primary->key_id, secondary ? secondary->key_id : -1);
186 186
 
187
-        int ret = dco_swap_keys(dco, multi->peer_id);
187
+        int ret = dco_swap_keys(dco, multi->dco_peer_id);
188 188
         if (ret < 0)
189 189
         {
190 190
             msg(D_DCO, "Cannot swap keys: %s (%d)", strerror(-ret), ret);
... ...
@@ -202,7 +202,7 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
202 202
     /* if we have no secondary key anymore, inform DCO about it */
203 203
     if (!secondary && multi->dco_keys_installed == 2)
204 204
     {
205
-        int ret = dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY);
205
+        int ret = dco_del_key(dco, multi->dco_peer_id, OVPN_KEY_SLOT_SECONDARY);
206 206
         if (ret < 0)
207 207
         {
208 208
             msg(D_DCO, "Cannot delete secondary key: %s (%d)", strerror(-ret), ret);
... ...
@@ -465,7 +465,7 @@ dco_p2p_add_new_peer(struct context *c)
465 465
         return ret;
466 466
     }
467 467
 
468
-    c->c2.tls_multi->dco_peer_added = true;
468
+    c->c2.tls_multi->dco_peer_id = multi->peer_id;
469 469
     c->c2.link_socket->info.lsa->actual.dco_installed = true;
470 470
 
471 471
     return 0;
... ...
@@ -479,10 +479,10 @@ dco_remove_peer(struct context *c)
479 479
         return;
480 480
     }
481 481
 
482
-    if (c->c1.tuntap && c->c2.tls_multi && c->c2.tls_multi->dco_peer_added)
482
+    if (c->c1.tuntap && c->c2.tls_multi && c->c2.tls_multi->dco_peer_id != -1)
483 483
     {
484
-        dco_del_peer(&c->c1.tuntap->dco, c->c2.tls_multi->peer_id);
485
-        c->c2.tls_multi->dco_peer_added = false;
484
+        dco_del_peer(&c->c1.tuntap->dco, c->c2.tls_multi->dco_peer_id);
485
+        c->c2.tls_multi->dco_peer_id = -1;
486 486
     }
487 487
 }
488 488
 
... ...
@@ -585,7 +585,7 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi)
585 585
         return ret;
586 586
     }
587 587
 
588
-    c->c2.tls_multi->dco_peer_added = true;
588
+    c->c2.tls_multi->dco_peer_id = peer_id;
589 589
 
590 590
     if (c->mode == CM_CHILD_TCP)
591 591
     {
... ...
@@ -1734,7 +1734,7 @@ process_outgoing_link(struct context *c)
1734 1734
                 if (should_use_dco_socket(c->c2.to_link_addr))
1735 1735
                 {
1736 1736
                     size = dco_do_write(&c->c1.tuntap->dco,
1737
-                                        c->c2.tls_multi->peer_id,
1737
+                                        c->c2.tls_multi->dco_peer_id,
1738 1738
                                         &c->c2.to_link);
1739 1739
                 }
1740 1740
                 else
... ...
@@ -2151,14 +2151,14 @@ do_deferred_options_part2(struct context *c)
2151 2151
         && (c->options.ping_send_timeout || c->c2.frame.mss_fix))
2152 2152
     {
2153 2153
         int ret = dco_set_peer(&c->c1.tuntap->dco,
2154
-                               c->c2.tls_multi->peer_id,
2154
+                               c->c2.tls_multi->dco_peer_id,
2155 2155
                                c->options.ping_send_timeout,
2156 2156
                                c->options.ping_rec_timeout,
2157 2157
                                c->c2.frame.mss_fix);
2158 2158
         if (ret < 0)
2159 2159
         {
2160 2160
             msg(D_DCO, "Cannot set parameters for DCO peer (id=%u): %s",
2161
-                c->c2.tls_multi->peer_id, strerror(-ret));
2161
+                c->c2.tls_multi->dco_peer_id, strerror(-ret));
2162 2162
             return false;
2163 2163
         }
2164 2164
     }
... ...
@@ -2305,6 +2305,42 @@ cleanup:
2305 2305
     return ret;
2306 2306
 }
2307 2307
 
2308
+static bool
2309
+multi_client_setup_dco_initial(struct multi_context *m,
2310
+                               struct multi_instance *mi,
2311
+                               struct gc_arena *gc)
2312
+{
2313
+    if (!dco_enabled(&mi->context.options))
2314
+    {
2315
+        /* DCO not enabled, nothing to do, return sucess */
2316
+        return true;
2317
+    }
2318
+    int ret = dco_multi_add_new_peer(m, mi);
2319
+    if (ret < 0)
2320
+    {
2321
+        msg(D_DCO, "Cannot add peer to DCO for %s: %s (%d)",
2322
+            multi_instance_string(mi, false, gc), strerror(-ret), ret);
2323
+        return false;
2324
+    }
2325
+
2326
+    if (mi->context.options.ping_send_timeout || mi->context.c2.frame.mss_fix)
2327
+    {
2328
+        ret = dco_set_peer(&mi->context.c1.tuntap->dco,
2329
+                           mi->context.c2.tls_multi->dco_peer_id,
2330
+                           mi->context.options.ping_send_timeout,
2331
+                           mi->context.options.ping_rec_timeout,
2332
+                           mi->context.c2.frame.mss_fix);
2333
+        if (ret < 0)
2334
+        {
2335
+            msg(D_DCO, "Cannot set DCO peer parameters for %s (id=%u): %s",
2336
+                multi_instance_string(mi, false, gc),
2337
+                mi->context.c2.tls_multi->dco_peer_id, strerror(-ret));
2338
+            return false;
2339
+        }
2340
+    }
2341
+    return true;
2342
+}
2343
+
2308 2344
 /**
2309 2345
  * Generates the data channel keys
2310 2346
  */
... ...
@@ -2432,39 +2468,16 @@ multi_client_connect_late_setup(struct multi_context *m,
2432 2432
     {
2433 2433
         mi->context.c2.tls_multi->multi_state = CAS_FAILED;
2434 2434
     }
2435
+    /* only continue if setting protocol options worked */
2436
+    else if (!multi_client_setup_dco_initial(m, mi, &gc))
2437
+    {
2438
+        mi->context.c2.tls_multi->multi_state = CAS_FAILED;
2439
+    }
2435 2440
     /* Generate data channel keys only if setting protocol options
2436
-     * has not failed */
2437
-    else
2441
+     * and DCO initial setup has not failed */
2442
+    else if (!multi_client_generate_tls_keys(&mi->context))
2438 2443
     {
2439
-        if (dco_enabled(&mi->context.options))
2440
-        {
2441
-            int ret = dco_multi_add_new_peer(m, mi);
2442
-            if (ret < 0)
2443
-            {
2444
-                msg(D_DCO, "Cannot add peer to DCO: %s (%d)", strerror(-ret), ret);
2445
-                mi->context.c2.tls_multi->multi_state = CAS_FAILED;
2446
-            }
2447
-
2448
-            if (mi->context.options.ping_send_timeout || mi->context.c2.frame.mss_fix)
2449
-            {
2450
-                int ret = dco_set_peer(&mi->context.c1.tuntap->dco,
2451
-                                       mi->context.c2.tls_multi->peer_id,
2452
-                                       mi->context.options.ping_send_timeout,
2453
-                                       mi->context.options.ping_rec_timeout,
2454
-                                       mi->context.c2.frame.mss_fix);
2455
-                if (ret < 0)
2456
-                {
2457
-                    msg(D_DCO, "Cannot set parameters for DCO peer (id=%u): %s",
2458
-                        mi->context.c2.tls_multi->peer_id, strerror(-ret));
2459
-                    mi->context.c2.tls_multi->multi_state = CAS_FAILED;
2460
-                }
2461
-            }
2462
-        }
2463
-
2464
-        if (!multi_client_generate_tls_keys(&mi->context))
2465
-        {
2466
-            mi->context.c2.tls_multi->multi_state = CAS_FAILED;
2467
-        }
2444
+        mi->context.c2.tls_multi->multi_state = CAS_FAILED;
2468 2445
     }
2469 2446
 
2470 2447
     /* send push reply if ready */
... ...
@@ -2472,7 +2485,6 @@ multi_client_connect_late_setup(struct multi_context *m,
2472 2472
     {
2473 2473
         process_incoming_push_request(&mi->context);
2474 2474
     }
2475
-
2476 2475
     gc_free(&gc);
2477 2476
 }
2478 2477
 
... ...
@@ -3226,8 +3238,8 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi,
3226 3226
     }
3227 3227
 
3228 3228
     /* When kernel already deleted the peer, the socket is no longer
3229
-     * installed and we don't need to cleanup the state in the kernel */
3230
-    mi->context.c2.tls_multi->dco_peer_added = false;
3229
+     * installed, and we do not need to clean up the state in the kernel */
3230
+    mi->context.c2.tls_multi->dco_peer_id = -1;
3231 3231
     mi->context.sig->signal_text = reason;
3232 3232
     multi_signal_instance(m, mi, SIGTERM);
3233 3233
 }
... ...
@@ -1315,6 +1315,7 @@ tls_multi_init(struct tls_options *tls_options)
1315 1315
 
1316 1316
     /* get command line derived options */
1317 1317
     ret->opt = *tls_options;
1318
+    ret->dco_peer_id = -1;
1318 1319
 
1319 1320
     return ret;
1320 1321
 }
... ...
@@ -661,7 +661,14 @@ struct tls_multi
661 661
     /* Only used when DCO is used to remember how many keys we installed
662 662
      * for this session */
663 663
     int dco_keys_installed;
664
-    bool dco_peer_added;
664
+    /**
665
+     * This is the handle that DCO uses to identify this session with the
666
+     * kernel.
667
+     *
668
+     * We keep this separate as the normal peer_id can change during
669
+     * p2p NCP and we need to track the id that is really used.
670
+     */
671
+    int dco_peer_id;
665 672
 
666 673
     dco_context_t *dco;
667 674
 };