Skip to content

Commit

Permalink
endpoint: Fix data races while accessing GetIdentity()
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@tgraf
tgraf committed Jun 8, 2020
1 parent e158b63 commit 10f4e2c
Show file tree
Hide file tree
Showing 10 changed files with 67 additions and 30 deletions.
5 changes: 5 additions & 0 deletions pkg/datapath/config.go
View file
Expand Up @@ -47,6 +47,11 @@ type LoadTimeConfiguration 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
Expand Down
4 changes: 2 additions & 2 deletions pkg/datapath/linux/config/config.go
View file
Expand Up @@ -482,7 +482,7 @@ func (h *HeaderfileWriter) writeStaticData(fw io.Writer, e datapath.EndpointConf

fmt.Fprint(fw, defineMAC("NODE_MAC", e.GetNodeMAC()))

secID := e.GetIdentity().Uint32()
secID := e.GetIdentityLocked().Uint32()
fmt.Fprintf(fw, defineUint32("SECLABEL", secID))
fmt.Fprintf(fw, defineUint32("SECLABEL_NB", byteorder.HostToNetwork(secID).(uint32)))
fmt.Fprintf(fw, defineUint32("POLICY_VERDICT_LOG_FILTER", e.GetPolicyVerdictLogFilter()))
Expand Down Expand Up @@ -558,7 +558,7 @@ func (h *HeaderfileWriter) writeTemplateConfig(fw *bufio.Writer, e datapath.Endp
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 (h *HeaderfileWriter) WriteTemplateConfig(w io.Writer, e datapath.EndpointConfiguration) error {
fw := bufio.NewWriter(w)
return h.writeTemplateConfig(fw, e)
Expand Down
7 changes: 7 additions & 0 deletions pkg/datapath/loader/template.go
View file
Expand Up @@ -101,6 +101,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 {
Expand Down
7 changes: 6 additions & 1 deletion pkg/endpoint/bpf.go
View file
Expand Up @@ -72,6 +72,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)

Expand Down Expand Up @@ -107,7 +109,7 @@ func (e *Endpoint) writeInformationalComments(w io.Writer) error {
" * NodeMAC: %s\n"+
" */\n\n",
e.IPv4.String(),
e.GetIdentity(), bpf.LocalMapName(policymap.MapName, e.ID),
e.getIdentity(), bpf.LocalMapName(policymap.MapName, e.ID),
e.nodeMAC)

fw.WriteString("/*\n")
Expand All @@ -126,6 +128,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{
Expand Down
7 changes: 6 additions & 1 deletion pkg/endpoint/cache.go
View file
Expand Up @@ -76,7 +76,7 @@ func (e *Endpoint) createEpInfoCache(epdir string) *epInfoCache {
id: e.GetID(),
ifName: e.ifName,
ipvlan: e.HasIpvlanDataPath(),
identity: e.GetIdentity(),
identity: e.getIdentity(),
mac: e.GetNodeMAC(),
ipv4: e.IPv4Address(),
ipv6: e.IPv6Address(),
Expand Down Expand Up @@ -131,6 +131,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)
Expand Down
16 changes: 15 additions & 1 deletion pkg/endpoint/endpoint.go
View file
Expand Up @@ -613,7 +613,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() identity.NumericIdentity {
return e.getIdentity()
}

// GetIdentity returns the numeric security identity of the endpoint
func (e *Endpoint) GetIdentity() identity.NumericIdentity {
e.unconditionalRLock()
defer e.runlock()

return e.getIdentity()
}

func (e *Endpoint) getIdentity() identity.NumericIdentity {
if e.SecurityIdentity != nil {
return e.SecurityIdentity.ID
}
Expand Down Expand Up @@ -2234,7 +2248,7 @@ func (e *Endpoint) GetProxyInfoByFields() (uint64, string, string, []string, str
if e.IsDisconnecting() {
err = fmt.Errorf("endpoint is in the process of being deleted")
}
return e.GetID(), e.GetIPv4Address(), e.GetIPv6Address(), e.GetLabels(), e.GetLabelsSHA(), uint64(e.GetIdentity()), err
return e.GetID(), e.GetIPv4Address(), e.GetIPv6Address(), e.GetLabels(), e.GetLabelsSHA(), uint64(e.getIdentity()), err
}

// WaitForFirstRegeneration waits for specific conditions before returning:
Expand Down
2 changes: 1 addition & 1 deletion pkg/envoy/server.go
View file
Expand Up @@ -1054,7 +1054,7 @@ func (s *XDSServer) UpdateNetworkPolicy(ep logger.EndpointUpdater, policy *polic
if ip == "" {
continue
}
networkPolicy := getNetworkPolicy(ip, ep.GetIdentity(), ep.ConntrackNameLocked(), policy, npMap,
networkPolicy := getNetworkPolicy(ip, ep.GetIdentityLocked(), ep.ConntrackNameLocked(), policy, npMap,
ingressPolicyEnforced, egressPolicyEnforced)
err := networkPolicy.Validate()
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/proxy/logger/epinfo.go
View file
Expand Up @@ -28,7 +28,7 @@ type EndpointInfoSource interface {
GetID() uint64
GetIPv4Address() string
GetIPv6Address() string
GetIdentity() identity.NumericIdentity
GetIdentityLocked() identity.NumericIdentity
GetLabels() []string
GetLabelsSHA() string
HasSidecarProxy() bool
Expand Down
16 changes: 8 additions & 8 deletions pkg/proxy/mock_test.go
View file
Expand Up @@ -35,19 +35,19 @@ type proxyUpdaterMock struct {
}

func (m *proxyUpdaterMock) GetProxyInfoByFields() (uint64, string, string, []string, string, uint64, error) {
return m.GetID(), m.GetIPv4Address(), m.GetIPv6Address(), m.GetLabels(), m.GetLabelsSHA(), uint64(m.GetIdentity()), nil
return m.GetID(), m.GetIPv4Address(), m.GetIPv6Address(), m.GetLabels(), m.GetLabelsSHA(), uint64(m.GetIdentityLocked()), nil
}

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) 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(npMap policy.NamedPortsMap, l4 *policy.L4Filter) (string, error) {
return "", nil
}
Expand Down
31 changes: 16 additions & 15 deletions pkg/testutils/endpoint.go
View file
Expand Up @@ -61,21 +61,22 @@ func NewTestHostEndpoint() 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) GetPolicyVerdictLogFilter() uint32 { return 0xffff }
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) IsHost() bool { return e.isHost }
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) GetPolicyVerdictLogFilter() uint32 { return 0xffff }
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) IsHost() bool { return e.isHost }

func (e *TestEndpoint) IPv4Address() addressing.CiliumIPv4 {
addr, _ := addressing.NewCiliumIPv4("192.0.2.3")
Expand Down

0 comments on commit 10f4e2c

Please sign in to comment.