endpoint: Fix data races while accessing GetIdentity()

[ upstream commit 10f4e2c ]

[ Backporter's notes: Several minor conflicts, simple resolution.
                      One extra previously-unnecessary function call
		      change in pkg/proxy/epinfo.go. ]

Calls to GetIdentity() have been assuming both that the endpoint is
locked or not locked.

* daemon/cmd/fqdn.go / notifyOnDNSMsg():

  LookupEndpointIDByIP(). No locking.

  --> Vulnerable

* pkg/datapath/linux/config/config.go / writeStaticData()

  e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData()

  The endpoint is locked. Not using epCacheInfo. Must use a non-locking
  variation or a deadlock may occur.

  --> Not vulnerable

* pkg/datapath/loader/template.go

  e.realizeBPFState() ->
  -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions()
  -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() ->
  -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions()

  Uses epInfoCache

  --> Not vulnerable

* pkg/envoy/server.go

  e.updateNetworkPolicy() -> UpdateNetworkPolicy()

  The endpoint is locked. Must use a non-locking variation.

  --> Not vulnerable

* pkg/hubble/parser/threefour/parser.go

  Decode() -> resolveEndpoint()

  --> Vulnerable

Fixes: #11932

Signed-off-by: Thomas Graf <thomas@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>

pkg/datapath/config.go

@@ -47,6 +47,11 @@ type EndpointConfiguration interface {
 	// GetIdentity returns a globally-significant numeric security identity.
 	GetIdentity() identity.NumericIdentity
 
+	// GetIdentityLocked returns a globally-significant numeric security
+	// identity while assuming that the backing data structure is locked.
+	// This function should be removed in favour of GetIdentity()
+	GetIdentityLocked() identity.NumericIdentity
+
 	IPv4Address() addressing.CiliumIPv4
 	IPv6Address() addressing.CiliumIPv6
 	GetNodeMAC() mac.MAC

pkg/datapath/linux/config.go

@@ -331,7 +331,7 @@ func (l *linuxDatapath) writeStaticData(fw io.Writer, e datapath.EndpointConfigu
 	fmt.Fprint(fw, defineMAC("NODE_MAC", e.GetNodeMAC()))
 	fmt.Fprint(fw, defineUint32("LXC_ID", uint32(e.GetID())))
 
-	secID := e.GetIdentity().Uint32()
+	secID := e.GetIdentityLocked().Uint32()
 	fmt.Fprintf(fw, defineUint32("SECLABEL", secID))
 	fmt.Fprintf(fw, defineUint32("SECLABEL_NB", byteorder.HostToNetwork(secID).(uint32)))
 
@@ -395,7 +395,7 @@ func (l *linuxDatapath) writeTemplateConfig(fw *bufio.Writer, e datapath.Endpoin
 	return fw.Flush()
 }
 
-// WriteEndpointConfig writes the BPF configuration for the template to a writer.
+// WriteTemplateConfig writes the BPF configuration for the template to a writer.
 func (l *linuxDatapath) WriteTemplateConfig(w io.Writer, e datapath.EndpointConfiguration) error {
 	fw := bufio.NewWriter(w)
 	return l.writeTemplateConfig(fw, e)

pkg/datapath/loader/template.go

@@ -93,6 +93,13 @@ func (t *templateCfg) GetIdentity() identity.NumericIdentity {
 	return TemplateSecurityID
 }
 
+// GetIdentityLocked is identical to GetIdentity(). This is a temporary
+// function until WriteEndpointConfig() no longer assumes that the endpoint is
+// locked.
+func (t *templateCfg) GetIdentityLocked() identity.NumericIdentity {
+	return TemplateSecurityID
+}
+
 // GetNodeMAC returns a well-known dummy MAC address which may be later
 // substituted in the ELF.
 func (t *templateCfg) GetNodeMAC() mac.MAC {

pkg/endpoint/bpf.go

@@ -77,6 +77,8 @@ func (e *Endpoint) BPFIpvlanMapPath() string {
 // strings describing the configuration of the datapath.
 //
 // For configuration of actual datapath behavior, see WriteEndpointConfig().
+//
+// e.Mutex must be held
 func (e *Endpoint) writeInformationalComments(w io.Writer) error {
 	fw := bufio.NewWriter(w)
 
@@ -110,7 +112,7 @@ func (e *Endpoint) writeInformationalComments(w io.Writer) error {
 		" * NodeMAC: %s\n"+
 		" */\n\n",
 		e.IPv6.String(), e.IPv4.String(),
-		e.GetIdentity(), bpf.LocalMapName(policymap.MapName, e.ID),
+		e.getIdentity(), bpf.LocalMapName(policymap.MapName, e.ID),
 		e.NodeMAC)
 
 	fw.WriteString("/*\n")
@@ -129,6 +131,9 @@ func (e *Endpoint) writeInformationalComments(w io.Writer) error {
 	return fw.Flush()
 }
 
+// writeHeaderfile writes the lxc_config.h header file of an endpoint
+//
+// e.Mutex must be held.
 func (e *Endpoint) writeHeaderfile(prefix string) error {
 	headerPath := filepath.Join(prefix, common.CHeaderFileName)
 	e.getLogger().WithFields(logrus.Fields{

pkg/endpoint/cache.go

@@ -81,7 +81,7 @@ func (e *Endpoint) createEpInfoCache(epdir string) *epInfoCache {
 		ifName: e.IfName,
 		ipvlan: e.HasIpvlanDataPath(),
 
-		identity:              e.GetIdentity(),
+		identity:              e.getIdentity(),
 		mac:                   e.GetNodeMAC(),
 		ipv4:                  e.IPv4Address(),
 		ipv6:                  e.IPv6Address(),
@@ -132,6 +132,11 @@ func (ep *epInfoCache) GetIdentity() identity.NumericIdentity {
 	return ep.identity
 }
 
+// GetIdentityLocked returns the security identity of the endpoint.
+func (ep *epInfoCache) GetIdentityLocked() identity.NumericIdentity {
+	return ep.identity
+}
+
 // Logger returns the logger for the endpoint that is being cached.
 func (ep *epInfoCache) Logger(subsystem string) *logrus.Entry {
 	return ep.endpoint.Logger(subsystem)

pkg/endpoint/endpoint.go

@@ -928,7 +928,21 @@ func (e *Endpoint) StringID() string {
 	return strconv.Itoa(int(e.ID))
 }
 
+// GetIdentityLocked is identical to GetIdentity() but assumes that a.mutex is
+// already held. This function is obsolete and should no longer be used.
+func (e *Endpoint) GetIdentityLocked() identityPkg.NumericIdentity {
+	return e.getIdentity()
+}
+
+// GetIdentity returns the numeric security identity of the endpoint
 func (e *Endpoint) GetIdentity() identityPkg.NumericIdentity {
+	e.UnconditionalRLock()
+	defer e.RUnlock()
+
+	return e.getIdentity()
+}
+
+func (e *Endpoint) getIdentity() identityPkg.NumericIdentity {
 	if e.SecurityIdentity != nil {
 		return e.SecurityIdentity.ID
 	}

pkg/endpoint/regeneration/owner.go

@@ -80,7 +80,7 @@ type EndpointInfoSource interface {
 	GetID() uint64
 	GetIPv4Address() string
 	GetIPv6Address() string
-	GetIdentity() identity.NumericIdentity
+	GetIdentityLocked() identity.NumericIdentity
 	GetLabels() []string
 	GetLabelsSHA() string
 	HasSidecarProxy() bool

pkg/envoy/server.go

@@ -797,7 +797,7 @@ func (s *XDSServer) UpdateNetworkPolicy(ep logger.EndpointUpdater, policy *polic
 		if ip == "" {
 			continue
 		}
-		networkPolicy := getNetworkPolicy(ip, ep.GetIdentity(), ep.ConntrackName(), policy,
+		networkPolicy := getNetworkPolicy(ip, ep.GetIdentityLocked(), ep.ConntrackName(), policy,
 			ingressPolicyEnforced, egressPolicyEnforced)
 		err := networkPolicy.Validate()
 		if err != nil {

pkg/proxy/epinfo.go

@@ -59,7 +59,7 @@ func (r *defaultEndpointInfoRegistry) FillEndpointIdentityByIP(ip net.IP, info *
 	}
 
 	info.ID = uint64(ep.ID)
-	info.Identity = uint64(ep.GetIdentity())
+	info.Identity = uint64(ep.GetIdentityLocked())
 	info.Labels = ep.GetLabels()
 	info.LabelsSHA256 = ep.GetLabelsSHA()
 

pkg/proxy/logger/epinfo.go

@@ -30,7 +30,7 @@ type EndpointInfoSource interface {
 	GetID() uint64
 	GetIPv4Address() string
 	GetIPv6Address() string
-	GetIdentity() identity.NumericIdentity
+	GetIdentityLocked() identity.NumericIdentity
 	GetLabels() []string
 	GetLabelsSHA() string
 	HasSidecarProxy() bool
@@ -51,7 +51,7 @@ func getEndpointInfo(source EndpointInfoSource) *accesslog.EndpointInfo {
 		IPv6:         source.GetIPv6Address(),
 		Labels:       source.GetLabels(),
 		LabelsSHA256: source.GetLabelsSHA(),
-		Identity:     uint64(source.GetIdentity()),
+		Identity:     uint64(source.GetIdentityLocked()),
 	}
 }
 

pkg/proxy/mock_test.go

@@ -37,14 +37,14 @@ type proxyUpdaterMock struct {
 func (m *proxyUpdaterMock) UnconditionalRLock() { m.RWMutex.RLock() }
 func (m *proxyUpdaterMock) RUnlock()            { m.RWMutex.RUnlock() }
 
-func (m *proxyUpdaterMock) GetID() uint64                         { return m.id }
-func (m *proxyUpdaterMock) GetIPv4Address() string                { return m.ipv4 }
-func (m *proxyUpdaterMock) GetIPv6Address() string                { return m.ipv6 }
-func (m *proxyUpdaterMock) GetLabels() []string                   { return m.labels }
-func (m *proxyUpdaterMock) GetEgressPolicyEnabledLocked() bool    { return true }
-func (m *proxyUpdaterMock) GetIngressPolicyEnabledLocked() bool   { return true }
-func (m *proxyUpdaterMock) GetIdentity() identity.NumericIdentity { return m.identity }
-func (m *proxyUpdaterMock) ProxyID(l4 *policy.L4Filter) string    { return "" }
+func (m *proxyUpdaterMock) GetID() uint64                               { return m.id }
+func (m *proxyUpdaterMock) GetIPv4Address() string                      { return m.ipv4 }
+func (m *proxyUpdaterMock) GetIPv6Address() string                      { return m.ipv6 }
+func (m *proxyUpdaterMock) GetLabels() []string                         { return m.labels }
+func (m *proxyUpdaterMock) GetEgressPolicyEnabledLocked() bool          { return true }
+func (m *proxyUpdaterMock) GetIngressPolicyEnabledLocked() bool         { return true }
+func (m *proxyUpdaterMock) GetIdentityLocked() identity.NumericIdentity { return m.identity }
+func (m *proxyUpdaterMock) ProxyID(l4 *policy.L4Filter) string          { return "" }
 func (m *proxyUpdaterMock) GetLabelsSHA() string {
 	return labels.NewLabelsFromModel(m.labels).SHA256Sum()
 }

pkg/testutils/endpoint.go

@@ -49,19 +49,20 @@ func NewTestEndpoint() TestEndpoint {
 	}
 }
 
-func (e *TestEndpoint) HasIpvlanDataPath() bool                 { return false }
-func (e *TestEndpoint) ConntrackLocalLocked() bool              { return false }
-func (e *TestEndpoint) RequireARPPassthrough() bool             { return false }
-func (e *TestEndpoint) RequireEgressProg() bool                 { return false }
-func (e *TestEndpoint) RequireRouting() bool                    { return false }
-func (e *TestEndpoint) RequireEndpointRoute() bool              { return false }
-func (e *TestEndpoint) GetCIDRPrefixLengths() ([]int, []int)    { return nil, nil }
-func (e *TestEndpoint) GetID() uint64                           { return e.Id }
-func (e *TestEndpoint) StringID() string                        { return "42" }
-func (e *TestEndpoint) GetIdentity() identity.NumericIdentity   { return e.Identity.ID }
-func (e *TestEndpoint) GetSecurityIdentity() *identity.Identity { return e.Identity }
-func (e *TestEndpoint) GetNodeMAC() mac.MAC                     { return e.MAC }
-func (e *TestEndpoint) GetOptions() *option.IntOptions          { return e.Opts }
+func (e *TestEndpoint) HasIpvlanDataPath() bool                     { return false }
+func (e *TestEndpoint) ConntrackLocalLocked() bool                  { return false }
+func (e *TestEndpoint) RequireARPPassthrough() bool                 { return false }
+func (e *TestEndpoint) RequireEgressProg() bool                     { return false }
+func (e *TestEndpoint) RequireRouting() bool                        { return false }
+func (e *TestEndpoint) RequireEndpointRoute() bool                  { return false }
+func (e *TestEndpoint) GetCIDRPrefixLengths() ([]int, []int)        { return nil, nil }
+func (e *TestEndpoint) GetID() uint64                               { return e.Id }
+func (e *TestEndpoint) StringID() string                            { return "42" }
+func (e *TestEndpoint) GetIdentity() identity.NumericIdentity       { return e.Identity.ID }
+func (e *TestEndpoint) GetIdentityLocked() identity.NumericIdentity { return e.Identity.ID }
+func (e *TestEndpoint) GetSecurityIdentity() *identity.Identity     { return e.Identity }
+func (e *TestEndpoint) GetNodeMAC() mac.MAC                         { return e.MAC }
+func (e *TestEndpoint) GetOptions() *option.IntOptions              { return e.Opts }
 
 func (e *TestEndpoint) IPv4Address() addressing.CiliumIPv4 {
 	addr, _ := addressing.NewCiliumIPv4("192.0.2.3")