Skip to content

cilium: encryption, segfaults if existing non-Cilium xfrm policy without mark set exists #10268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 20, 2020
Merged

cilium: encryption, segfaults if existing non-Cilium xfrm policy without mark set exists #10268

merged 1 commit into from
Feb 20, 2020

Conversation

jrfastab
Copy link
Contributor

@jrfastab jrfastab commented Feb 20, 2020 •
edited by tgraf
Loading

If an existing encryption (ipsec) is in place and that policy does not
have a mark field in the netlink message set then the policy list API
will have a nil mark pointer.

However, we immediately dereference that pointer to check if its a
Cilium installed policy without checking for nil first. The result
is the following segfault.

[signal SIGSEGV: segmentation violation code=0x1 addr=0x4 pc=0x1c853df]
goroutine 1 [running]:
github.com/cilium/cilium/pkg/datapath/linux/ipsec.isXfrmPolicyCilium(...)
/go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:367
github.com/cilium/cilium/pkg/datapath/linux/ipsec.DeleteXfrm()
/go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:393 +0x10f
github.com/cilium/cilium/pkg/datapath/linux.(*linuxNodeHandler).NodeConfigur

Add nil pointer checks here to resolve this.

Signed-off-by: John Fastabend john.fastabend@gmail.com

This change is Reviewable

@jrfastab
cilium: encryption, segfault if existing xfrm policy without mark set
c07670f 
If an existing encryption (ipsec) is in place and that policy does not
have a mark field in the netlink message set then the policy list API
will have a nil mark pointer.

However, we immediately dereference that pointer to check if its a
Cilium installed policy without checking for nil first. The result
is the following segfault.

[signal SIGSEGV: segmentation violation code=0x1 addr=0x4 pc=0x1c853df]
goroutine 1 [running]:
github.com/cilium/cilium/pkg/datapath/linux/ipsec.isXfrmPolicyCilium(...)
        /go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:367
github.com/cilium/cilium/pkg/datapath/linux/ipsec.DeleteXfrm()
        /go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:393 +0x10f
github.com/cilium/cilium/pkg/datapath/linux.(*linuxNodeHandler).NodeConfigur

Add nil pointer checks here to resolve this.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab jrfastab added kind/bug This is a bug in the Cilium logic. needs-backport/1.6 labels Feb 20, 2020
@jrfastab jrfastab requested a review from a team February 20, 2020 19:47
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

2 similar comments
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@jrfastab
Copy link
Contributor Author

test-me-please

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 45.511% when pulling c07670f on encrypt-segfault into 01fb5de on master.

@joestringer joestringer added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Feb 20, 2020
@joestringer joestringer merged commit 7fd7d42 into master Feb 20, 2020
@joestringer joestringer deleted the encrypt-segfault branch February 20, 2020 22:26
@nebril nebril mentioned this pull request Feb 21, 2020
Merged
@nebril nebril mentioned this pull request Feb 21, 2020
Merged
@brb brb mentioned this pull request Feb 27, 2020
Merged
@joestringer
Copy link
Member

Full example of output from a Slack user:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x4 pc=0x1c853df]
goroutine 1 [running]:
github.com/cilium/cilium/pkg/datapath/linux/ipsec.isXfrmPolicyCilium(...)
	/go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:367
github.com/cilium/cilium/pkg/datapath/linux/ipsec.DeleteXfrm()
	/go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:393 +0x10f
github.com/cilium/cilium/pkg/datapath/linux.(*linuxNodeHandler).NodeConfigurationChanged(0xc000a9a0c0, 0x5dc, 0x5aa, 0x5dc, 0x1, 0x3f928b0, 0x0, 0x0, 0x10001000001, 0x3f928b0, ...)
	/go/src/github.com/cilium/cilium/pkg/datapath/linux/node.go:1102 +0x69a
github.com/cilium/cilium/pkg/datapath/loader.(*Loader).Reinitialize(0xc000550360, 0x274cbc0, 0xc000119620, 0x2764c00, 0xc00048f680, 0x5dc, 0x7f0cbf24f998, 0xc0001b29a0, 0x26edf40, 0xc000130cd0, ...)
	/go/src/github.com/cilium/cilium/pkg/datapath/loader/base.go:284 +0xf55
main.(*Daemon).init(0xc00048f680, 0x1700812c9, 0x3a49920)
	/go/src/github.com/cilium/cilium/daemon/daemon.go:202 +0x7c6
main.NewDaemon(0x274cb40, 0xc000325100, 0x278fe20, 0xc0001b29a0, 0x276a540, 0xc000550120, 0x278fe20, 0xc0001b29a0)
	/go/src/github.com/cilium/cilium/daemon/daemon.go:491 +0x1a62
main.runDaemon()
	/go/src/github.com/cilium/cilium/daemon/daemon_main.go:1181 +0x350
main.glob..func1(0x3a27040, 0xc0003e3e90, 0x0, 0x1)
	/go/src/github.com/cilium/cilium/daemon/daemon_main.go:101 +0x7b
github.com/spf13/cobra.(*Command).execute(0x3a27040, 0xc00010a030, 0x1, 0x1, 0x3a27040, 0xc00010a030)
	/go/src/github.com/cilium/cilium/vendor/github.com/spf13/cobra/command.go:830 +0x2aa
github.com/spf13/cobra.(*Command).ExecuteC(0x3a27040, 0xc0000f2ae0, 0x0, 0x0)
	/go/src/github.com/cilium/cilium/vendor/github.com/spf13/cobra/command.go:914 +0x2fb
github.com/spf13/cobra.(*Command).Execute(...)
	/go/src/github.com/cilium/cilium/vendor/github.com/spf13/cobra/command.go:864
main.daemonMain()
	/go/src/github.com/cilium/cilium/daemon/daemon_main.go:129 +0x184
main.main()
	/go/src/github.com/cilium/cilium/daemon/main.go:18 +0x20

@jrajahalme jrajahalme mentioned this pull request Mar 4, 2020
Merged
@vadorovsky
Copy link
Member

I don't think it's possible to backport wthout #9649

This was referenced Mar 4, 2020
Merged
Merged
@rolinh
Copy link
Member

rolinh commented Mar 19, 2020

I don't think it's possible to backport wthout #9649

Indeed, can't backport to v1.6 without this one.

@jrfastab jrfastab mentioned this pull request Mar 19, 2020
Closed
@jrfastab jrfastab mentioned this pull request Apr 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jrfastab @coveralls @joestringer @vadorovsky @rolinh @nebril