[src/trunk]: src/sys/netipsec Avoid updating sav directly

[ozaki-r <ozaki-r%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/725a3a9b276e
branches:  trunk
changeset: 354960:725a3a9b276e
user:      ozaki-r <ozaki-r%NetBSD.org@localhost>
date:      Thu Jul 06 09:48:42 2017 +0000

description:
Avoid updating sav directly

On SADB_UPDATE a target sav was updated directly, which was unsafe.
Instead allocate another sav, copy variables of the old sav to
the new one and replace the old one with the new one.

diffstat:

 sys/netipsec/key.c |  46 ++++++++++++++++++++++++++++++++++++----------
 1 files changed, 36 insertions(+), 10 deletions(-)

diffs (92 lines):

diff -r f9a4053ad2a6 -r 725a3a9b276e sys/netipsec/key.c
--- a/sys/netipsec/key.c        Thu Jul 06 09:24:47 2017 +0000
+++ b/sys/netipsec/key.c        Thu Jul 06 09:48:42 2017 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: key.c,v 1.166 2017/07/06 09:04:26 ozaki-r Exp $        */
+/*     $NetBSD: key.c,v 1.167 2017/07/06 09:48:42 ozaki-r Exp $        */
 /*     $FreeBSD: src/sys/netipsec/key.c,v 1.3.2.3 2004/02/14 22:23:23 bms Exp $        */
 /*     $KAME: key.c,v 1.191 2001/06/27 10:46:49 sakane Exp $   */
 
@@ -32,7 +32,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: key.c,v 1.166 2017/07/06 09:04:26 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: key.c,v 1.167 2017/07/06 09:48:42 ozaki-r Exp $");
 
 /*
  * This code is referd to RFC 2367
@@ -3150,6 +3150,9 @@
        KASSERT(mhp != NULL);
        KASSERT(mhp->msg != NULL);
 
+       /* We shouldn't initialize sav variables while someone uses it. */
+       KASSERT(sav->refcnt == 0);
+
        /* initialization */
        key_freesaval(sav);
        sav->tdb_xform = NULL;          /* transform */
@@ -5176,7 +5179,7 @@
        const struct sockaddr *src, *dst;
        struct secasindex saidx;
        struct secashead *sah;
-       struct secasvar *sav;
+       struct secasvar *sav, *newsav;
        u_int16_t proto;
        u_int8_t mode;
        u_int16_t reqid;
@@ -5283,24 +5286,47 @@
                return key_senderror(so, m, EINVAL);
        }
 
+       /*
+        * Allocate a new SA instead of modifying the existing SA directly
+        * to avoid race conditions.
+        */
+       newsav = kmem_zalloc(sizeof(struct secasvar), KM_SLEEP);
+
        /* copy sav values */
-       error = key_setsaval(sav, m, mhp);
+       newsav->spi = sav->spi;
+       newsav->seq = sav->seq;
+       newsav->created = sav->created;
+       newsav->pid = sav->pid;
+
+       error = key_setsaval(newsav, m, mhp);
        if (error) {
-               KEY_FREESAV(&sav);
+               KEY_FREESAV(&newsav);
                return key_senderror(so, m, error);
        }
 
-       error = key_handle_natt_info(sav,mhp);
-       if (error != 0)
-               return key_senderror(so, m, EINVAL);
+       error = key_handle_natt_info(newsav, mhp);
+       if (error != 0) {
+               KEY_FREESAV(&newsav);
+               return key_senderror(so, m, error);
+       }
+
+       /* add to satree */
+       newsav->sah = sah;
+       newsav->refcnt = 1;
+       newsav->state = sav->state;
+       /* We have to keep the order */
+       LIST_INSERT_AFTER(sav, newsav, chain);
 
        /* check SA values to be mature. */
-       error = key_mature(sav);
+       error = key_mature(newsav);
        if (error != 0) {
-               KEY_FREESAV(&sav);
+               KEY_FREESAV(&newsav);
                return key_senderror(so, m, error);
        }
 
+       key_sa_chgstate(sav, SADB_SASTATE_DEAD);
+       KEY_FREESAV(&sav);
+
     {
        struct mbuf *n;