libn/d/overlay: fix encryption race conditions

corhere · corhere · commit 843cd96725eb · 2025-07-09T14:12:26.000-04:00
There is a dedicated mutex for synchronizing access to the encrMap.
Separately, the main driver mutex is used for synchronizing access to
the encryption keys. Their use is sufficient to prevent data races (if
used correctly, which is not the case) but not logical race conditions.
Programming the encryption parameters for a peer can race with
encryption keys being updated, which could lead to inconsistencies
between the parameters programmed into the kernel and the desired state.

Introduce a new mutex for synchronizing encryption operations. Use that
mutex to synchronize access to both encrMap and keys. Handle encryption
key updates in a critical section so they can no longer be interleaved
with kernel programming of encryption parameters.

Signed-off-by: Cory Snider &lt;csnider@mirantis.com&gt;
diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go
@@ -13,7 +13,6 @@ import (
 	"net"
 	"net/netip"
 	"strconv"
-	"sync"
 	"syscall"
 
 	"github.com/containerd/log"
@@ -96,16 +95,14 @@ type encrNode struct {
 	count int
 }
 
-type encrMap struct {
-	nodes map[netip.Addr]encrNode
-	mu    sync.Mutex
-}
+// encrMap is a map of node IP addresses to their encryption parameters.
+//
+// Like all Go maps, it is not safe for concurrent use.
+type encrMap map[netip.Addr]encrNode
 
-func (e *encrMap) String() string {
-	e.mu.Lock()
-	defer e.mu.Unlock()
+func (e encrMap) String() string {
 	b := new(bytes.Buffer)
-	for k, v := range e.nodes {
+	for k, v := range e {
 		b.WriteString("\n")
 		b.WriteString(k.String())
 		b.WriteString(":")
@@ -124,16 +121,19 @@ func (e *encrMap) String() string {
 func (d *driver) setupEncryption(remoteIP netip.Addr) error {
 	log.G(context.TODO()).Debugf("setupEncryption(%s)", remoteIP)
 
-	localIP, advIP := d.bindAddress, d.advertiseAddress
-	keys := d.keys // FIXME: data race
-	if len(keys) == 0 {
+	d.encrMu.Lock()
+	defer d.encrMu.Unlock()
+	if len(d.keys) == 0 {
 		return types.ForbiddenErrorf("encryption key is not present")
 	}
+	d.mu.Lock()
+	localIP, advIP := d.bindAddress, d.advertiseAddress
+	d.mu.Unlock()
 	log.G(context.TODO()).Debugf("Programming encryption between %s and %s", localIP, remoteIP)
 
-	indices := make([]spi, 0, len(keys))
+	indices := make([]spi, 0, len(d.keys))
 
-	for i, k := range keys {
+	for i, k := range d.keys {
 		spis := spi{buildSPI(advIP.AsSlice(), remoteIP.AsSlice(), k.tag), buildSPI(remoteIP.AsSlice(), advIP.AsSlice(), k.tag)}
 		dir := reverse
 		if i == 0 {
@@ -153,31 +153,29 @@ func (d *driver) setupEncryption(remoteIP netip.Addr) error {
 		}
 	}
 
-	d.secMap.mu.Lock()
-	node := d.secMap.nodes[remoteIP]
+	node := d.secMap[remoteIP]
 	node.spi = indices
 	node.count++
-	d.secMap.nodes[remoteIP] = node
-	d.secMap.mu.Unlock()
+	d.secMap[remoteIP] = node
 
 	return nil
 }
 
 func (d *driver) removeEncryption(remoteIP netip.Addr) error {
 	log.G(context.TODO()).Debugf("removeEncryption(%s)", remoteIP)
 
-	spi := func() []spi {
-		d.secMap.mu.Lock()
-		defer d.secMap.mu.Unlock()
-		node := d.secMap.nodes[remoteIP]
-		if node.count == 1 {
-			delete(d.secMap.nodes, remoteIP)
-			return node.spi
-		}
+	d.encrMu.Lock()
+	defer d.encrMu.Unlock()
+
+	var spi []spi
+	node := d.secMap[remoteIP]
+	if node.count == 1 {
+		delete(d.secMap, remoteIP)
+		spi = node.spi
+	} else {
 		node.count--
-		d.secMap.nodes[remoteIP] = node
-		return nil
-	}()
+		d.secMap[remoteIP] = node
+	}
 
 	for i, idxs := range spi {
 		dir := reverse
@@ -452,20 +450,24 @@ func buildAeadAlgo(k *key, s int) *netlink.XfrmStateAlgo {
 }
 
 func (d *driver) setKeys(keys []*key) error {
+	d.encrMu.Lock()
+	defer d.encrMu.Unlock()
+
 	// Remove any stale policy, state
 	clearEncryptionStates()
 	// Accept the encryption keys and clear any stale encryption map
-	d.mu.Lock()
+	d.secMap = encrMap{}
 	d.keys = keys
-	d.secMap = &encrMap{nodes: map[netip.Addr]encrNode{}}
-	d.mu.Unlock()
 	log.G(context.TODO()).Debugf("Initial encryption keys: %v", keys)
 	return nil
 }
 
 // updateKeys allows to add a new key and/or change the primary key and/or prune an existing key
 // The primary key is the key used in transmission and will go in first position in the list.
 func (d *driver) updateKeys(newKey, primary, pruneKey *key) error {
+	d.encrMu.Lock()
+	defer d.encrMu.Unlock()
+
 	log.G(context.TODO()).Debugf("Updating Keys. New: %v, Primary: %v, Pruned: %v", newKey, primary, pruneKey)
 
 	log.G(context.TODO()).Debugf("Current: %v", d.keys)
@@ -478,9 +480,6 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error {
 		aIP    = d.advertiseAddress
 	)
 
-	d.mu.Lock()
-	defer d.mu.Unlock()
-
 	// add new
 	if newKey != nil {
 		d.keys = append(d.keys, newKey)
@@ -506,14 +505,12 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error {
 		return types.InvalidParameterErrorf("attempting to both make a key (index %d) primary and delete it", priIdx)
 	}
 
-	d.secMap.mu.Lock()
-	for rIP, node := range d.secMap.nodes {
+	for rIP, node := range d.secMap {
 		idxs := updateNodeKey(lIP.AsSlice(), aIP.AsSlice(), rIP.AsSlice(), node.spi, d.keys, newIdx, priIdx, delIdx)
 		if idxs != nil {
-			d.secMap.nodes[rIP] = encrNode{idxs, node.count}
+			d.secMap[rIP] = encrNode{idxs, node.count}
 		}
 	}
-	d.secMap.mu.Unlock()
 
 	// swap primary
 	if priIdx != -1 {
diff --git a/libnetwork/drivers/overlay/joinleave.go b/libnetwork/drivers/overlay/joinleave.go
@@ -45,8 +45,13 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
 		return fmt.Errorf("could not find endpoint with id %s", eid)
 	}
 
-	if n.secure && len(d.keys) == 0 {
-		return errors.New("cannot join secure network: encryption keys not present")
+	if n.secure {
+		d.encrMu.Lock()
+		nkeys := len(d.keys)
+		d.encrMu.Unlock()
+		if nkeys == 0 {
+			return errors.New("cannot join secure network: encryption keys not present")
+		}
 	}
 
 	nlh := ns.NlHandle()
diff --git a/libnetwork/drivers/overlay/overlay.go b/libnetwork/drivers/overlay/overlay.go
@@ -31,12 +31,17 @@ var _ discoverapi.Discover = (*driver)(nil)
 type driver struct {
 	bindAddress, advertiseAddress netip.Addr
 
+	// encrMu guards secMap and keys,
+	// and synchronizes the application of encryption parameters
+	// to the kernel.
+	encrMu sync.Mutex
+	secMap encrMap
+	keys   []*key
+
 	config   map[string]interface{}
 	peerDb   peerNetworkMap
-	secMap   *encrMap
 	networks networkTable
 	initOS   sync.Once
-	keys     []*key
 	peerOpMu sync.Mutex
 	mu       sync.Mutex
 }
@@ -48,7 +53,7 @@ func Register(r driverapi.Registerer, config map[string]interface{}) error {
 		peerDb: peerNetworkMap{
 			mp: map[string]*peerMap{},
 		},
-		secMap: &encrMap{nodes: map[netip.Addr]encrNode{}},
+		secMap: encrMap{},
 		config: config,
 	}
 	return r.RegisterDriver(NetworkType, d, driverapi.Capability{

Original file line number	Diff line number	Diff line change
`@@ -45,8 +45,13 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf`
`45`	`45`	`return fmt.Errorf("could not find endpoint with id %s", eid)`
`46`	`46`	`}`
`47`	`47`
`48`		`- if n.secure && len(d.keys) == 0 {`
`49`		`- return errors.New("cannot join secure network: encryption keys not present")`
	`48`	`+ if n.secure {`
	`49`	`+ d.encrMu.Lock()`
	`50`	`+ nkeys := len(d.keys)`
	`51`	`+ d.encrMu.Unlock()`
	`52`	`+ if nkeys == 0 {`
	`53`	`+ return errors.New("cannot join secure network: encryption keys not present")`
	`54`	`+ }`
`50`	`55`	`}`
`51`	`56`
`52`	`57`	`nlh := ns.NlHandle()`