`linux`: Support setting execution domain via linux `personality`. #3126

flouthoc · 2021-08-03T10:02:13Z

Hi Team,

Following PR allows users to configure execution domain via syscall.SYS_PERSONALITY.
Valid inputs supported via spec are limited to LINUX and LINUX32.

References:
https://man7.org/linux/man-pages/man2/personality.2.html
https://raw.githubusercontent.com/torvalds/linux/master/include/uapi/linux/personality.h

flouthoc · 2021-08-03T10:09:07Z

utils_linux.go

@@ -272,6 +273,13 @@ func (r *runner) run(config *specs.Process) (int, error) {
 	if err = r.checkTerminal(config); err != nil {
 		return -1, err
 	}
+	// configure linux personality
+	if r.container.Config().Personality != nil {
+		err = setLinuxPersonality(r.container.Config().Personality.Domain)


Should we fail for this err ? I low-key want to ignore this err if syscall.SYS_PERSONALITY fails proceed as if nothing happened. I'll probably wait for mods to suggest here.

Failing seems better for security.

I guess we should also fail when unknown Personality.Flags are set

@AkihiroSuda Cool i agree it makes sense from security perspective so i will let the check be there. For Personality.Flags i did not implement it as runtime-spec suggests that no flag values are supported as well as domain is limited to LINUX and LINUX32. So anything unknown defaults to LINUX i.e 0x0000.

libcontainer/configs/config.go

kolyshkin · 2021-08-03T18:27:50Z

@flouthoc can you describe the use case? I thought it's for starting i386 binaries but they work as is.

I think internally it's better to represent the personality value as a number rather than a string.

kolyshkin · 2021-08-03T18:32:39Z

This should shed some light: opencontainers/runtime-spec@5cc25d0

crun support: containers/crun@afc183b

flouthoc · 2021-08-03T20:59:09Z

@kolyshkin the commit you shared describes the edge-case but I also think that build for any i386 or 32-bit binary will fail if current kernel is on 64-bit also certain 32-bit userspace apps expect uname -m to report i686. Following cases could be resolved by setting LINUX32 execution domain.

Also added as comment here https://github.com/opencontainers/runc/pull/3126/files#diff-96def2e006cb3518f1aaf9a595a80648521905e352e625550bff15aaa0019fa0R404

I think internally it's better to represent the personality value as a number rather than a string.

For this personalty is defined as number const internally in runc but accepted as string via spec since that is how it is defined in upstream runtime-spec this would required change in upstream. Please correct me if i am wrong.

const (
	PER_LINUX   = 0x0000
	PER_LINUX32 = 0x0008
)

kolyshkin · 2021-08-03T23:26:03Z

It's kind of weird that

this is still not implemented
this is not implemented by checking the architecture of container init binary (so, say, if we're running i386 binary, runc calls personality(PER_LINUX32) for us.

But I guess it is what it is.

kolyshkin · 2021-08-03T23:28:53Z

For this personalty is defined as number const internally in runc but accepted as string via spec since that is how it is defined in upstream runtime-spec this would required change in upstream. Please correct me if i am wrong.

I didn't mean to change the runtime spec. What I meant is we're converting the spec into our internal presentation anyway (in libcontainer/specconv), and this is where, I think, we should do a string -> number translation.

flouthoc · 2021-08-04T05:09:38Z

@kolyshkin Applied requested changes could you please review again.

flouthoc · 2021-08-05T11:54:07Z

@AkihiroSuda @kolyshkin We would also need downstream pr on managers / shims to support this. containers/podman#11141

flouthoc · 2021-08-06T12:16:16Z

@kolyshkin anything else which needs to be done from my side.

utils_linux.go

kolyshkin

Couple of nits, and we need a test case.

flouthoc · 2021-08-11T11:03:39Z

@kolyshkin Could you please take a look again ? resolved the feedback comments and added integration tests. Also changed the approach a little bit.

Instead of invoking syscall from runner we are now invoking it from linuxcontainer.Start , since integration tests are invoking Start directly instead of using CLI also new approach seems much better to me since personality is limited only to linux containers.

flouthoc · 2021-08-11T19:35:51Z

@kolyshkin Squeezed personality tests with exec integration tests, let me know if separate file needs to created.

flouthoc · 2021-08-18T10:51:12Z

@kolyshkin were you able to take a look at this, is there anything else with needs to be done from my side

Signed-off-by: Aditya Rajan <flouthoc.git@gmail.com>

kolyshkin · 2021-08-31T01:37:11Z

libcontainer/container_linux.go

@@ -230,6 +230,13 @@ func (c *linuxContainer) Set(config configs.Config) error {
 func (c *linuxContainer) Start(process *Process) error {
 	c.m.Lock()
 	defer c.m.Unlock()
+	// configure linux personality before starting the process
+	if c.config.Personality != nil {
+		err := system.SetLinuxPersonality(c.config.Personality.Domain)


I'm still unsure this is the best place. @cyphar can you please take a look?

Since we want to set personality as soon as process starts so this seems the best place to me.

Well, here you set it before the process starts, not as soon as.

sorry for confusion i meant this should happen just before the process start or as soon as the process start so execution happens in configured execution domain. But i am cool if you think there is any better place for this call.

I'm not super familiar with personalities to be honest, my guess is that we would want to do this as close to execve as possible since I don't know what effect setting the personality will have to runc's internal code. As such, I would suggest this should probably happen as close to execve as possible -- that would mean in standard_init_linux.go and setns_init_linux.go -- right before we apply seccomp filters probably.

@flouthoc When talking about the "process start" -- this code is being run very early in container setup. Do you mean that we need to set this way before the users' code has even started executing (I would've thought we only need to set it right before we execve the users' binary)?

@cyphar I actually tried it in standard_init_linux.go a while ago for some reasons I don't exactly recall i was having some issues, something something on the lines of goroutines and because of it personality tests were failing. I'll try it again and post insights thanks.

cyphar · 2021-09-09T04:16:37Z

libcontainer/specconv/spec_linux.go

+// check libcontainer/configs/config_linux.go
+// check https://raw.githubusercontent.com/torvalds/linux/master/include/uapi/linux/personality.h


You don't need to add these references each time.

cyphar · 2021-09-09T04:17:04Z

libcontainer/specconv/spec_linux.go

+func getLinuxPersonalityFromStr(domain string) int {
+	// defaults to PER_LINUX
+	if domain == "LINUX32" {
+		return configs.PER_LINUX32
+	}
+	return configs.PER_LINUX
+}


I don't think we should ignore strange settings -- this is just going to cause nightmares for backwards compatibility. Since getLinuxPersonalityFromStr will only be called if a user has explicitly specified a personality, we should definitely return an error if the personality name is meaningless.

cyphar · 2021-09-09T04:17:14Z

libcontainer/integration/exec_test.go

+func TestPersonality(t *testing.T) {
+	if testing.Short() {
+		return
+	}
+
+	config := newTemplateConfig(t, &tParam{})
+
+	// change execution domain to i686
+	config.Personality = &configs.LinuxPersonality{
+		Domain: configs.PER_LINUX32,
+	}
+
+	out, _, err := runContainer(t, config, "", "/bin/sh", "-c", "uname -a")
+	ok(t, err)
+	// output must contain kernel architecture configured as i686
+	if !strings.Contains(out.Stdout.String(), "i686") {
+		t.Fatalf("expected kernel architecture i686 configured via personality")
+	}
+}
+


Just for future reference -- I would prefer a bats-based integration test, but since you've already written this one, don't worry about it.

cyphar · 2021-09-09T04:20:57Z

libcontainer/container_linux.go

@@ -230,6 +230,13 @@ func (c *linuxContainer) Set(config configs.Config) error {
 func (c *linuxContainer) Start(process *Process) error {
 	c.m.Lock()
 	defer c.m.Unlock()
+	// configure linux personality before starting the process
+	if c.config.Personality != nil {
+		err := system.SetLinuxPersonality(c.config.Personality.Domain)


I'm not super familiar with personalities to be honest, my guess is that we would want to do this as close to execve as possible since I don't know what effect setting the personality will have to runc's internal code. As such, I would suggest this should probably happen as close to execve as possible -- that would mean in standard_init_linux.go and setns_init_linux.go -- right before we apply seccomp filters probably.

@flouthoc When talking about the "process start" -- this code is being run very early in container setup. Do you mean that we need to set this way before the users' code has even started executing (I would've thought we only need to set it right before we execve the users' binary)?

AkihiroSuda · 2023-02-01T02:46:09Z

@flouthoc Could you update the PR?

kolyshkin · 2023-09-26T23:33:36Z

@flouthoc do you still want to work on this one? If yes, can you please rebase?

Zheaoli · 2023-09-27T03:04:30Z

@kolyshkin @AkihiroSuda @cyphar Hello guys, this PR seems been deprecated for a while. may I take handle of it and carry it?

AkihiroSuda · 2023-09-27T03:16:24Z

@kolyshkin @AkihiroSuda @cyphar Hello guys, this PR seems been deprecated for a while. may I take handle of it and carry it?

Yes, please

flouthoc · 2023-09-27T05:47:23Z

Closing this PR in favor of above discussion. @Zheaoli Could you please open a new PR for this, feel free to copy whatever code is needed as it is or change parts as per needs i'm not sure if its code in this PR is updated as per recent runc version but hope it works.

Zheaoli · 2023-09-27T06:19:42Z

Closing this PR in favor of above discussion. @Zheaoli Could you please open a new PR for this, feel free to copy whatever code is needed as it is or change parts as per needs i'm not sure if its code in this PR is updated as per recent runc version but hope it works.

Of course, thanks for your work!

AkihiroSuda · 2023-10-20T01:52:36Z

@Zheaoli Do you plan to open a new PR?

Zheaoli · 2023-10-20T02:53:53Z

@Zheaoli Do you plan to open a new PR?

Yes I will open a new PR this weekend

…a linux personality Signed-off-by: Zheao.Li <me@manjusaka.me>

…a linux personality Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

carry opencontainers#3126 Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

carry #3126: linux: Support setting execution domain via linux personality

flouthoc commented Aug 3, 2021

View reviewed changes

AkihiroSuda added impact/changelog kind/feature labels Aug 3, 2021

AkihiroSuda added this to the 1.1.0 milestone Aug 3, 2021

kolyshkin reviewed Aug 3, 2021

View reviewed changes

libcontainer/configs/config.go Outdated Show resolved Hide resolved

flouthoc force-pushed the linux-support-personality branch from e787337 to cb03125 Compare August 3, 2021 20:40

flouthoc requested review from kolyshkin and AkihiroSuda August 3, 2021 20:59

flouthoc force-pushed the linux-support-personality branch 2 times, most recently from a856ea2 to e6bcfd3 Compare August 4, 2021 05:03

flouthoc mentioned this pull request Aug 5, 2021

personality: Add support for setting execution domain. containers/podman#11141

Merged

kolyshkin reviewed Aug 10, 2021

View reviewed changes

utils_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed Aug 10, 2021

View reviewed changes

utils_linux.go Outdated Show resolved Hide resolved

kolyshkin previously approved these changes Aug 10, 2021

View reviewed changes

tianon mentioned this pull request Aug 10, 2021

Docker "--platform" broken for native sub-architectures moby/moby#42732

Open

flouthoc dismissed kolyshkin’s stale review via 050289d August 11, 2021 10:55

flouthoc force-pushed the linux-support-personality branch from e6bcfd3 to 050289d Compare August 11, 2021 10:55

flouthoc requested a review from kolyshkin August 11, 2021 11:04

flouthoc force-pushed the linux-support-personality branch from 050289d to 4600cdd Compare August 11, 2021 11:07

linux: Support setting execution domain via linux personality.

aab68b3

Signed-off-by: Aditya Rajan <flouthoc.git@gmail.com>

flouthoc force-pushed the linux-support-personality branch from 828567b to aab68b3 Compare August 23, 2021 06:38

kolyshkin reviewed Aug 31, 2021

View reviewed changes

cyphar requested changes Sep 9, 2021

View reviewed changes

cyphar modified the milestones: 1.1.0, 1.2.0 Sep 9, 2021

AkihiroSuda mentioned this pull request Feb 1, 2023

go:mod: runtime-spec v1.1.0-rc.1; add docs/spec-conformance.md #3716

Merged

kolyshkin modified the milestones: 1.2.0, 1.3.0 Sep 26, 2023

flouthoc closed this Sep 27, 2023

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 20, 2023

carry opencontainers#3126: linux: Support setting execution domain vi…

26d8ff4

…a linux personality Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli mentioned this pull request Oct 20, 2023

carry #3126: linux: Support setting execution domain via linux personality #4082

Merged

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 21, 2023

carry opencontainers#3126: linux: Support setting execution domain vi…

70a8baf

…a linux personality Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 21, 2023

carry opencontainers#3126: linux: Support setting execution domain vi…

36e24ca

…a linux personality Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 21, 2023

carry opencontainers#3126: linux: Support setting execution domain vi…

8b5ab03

…a linux personality Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 21, 2023

linux: Support setting execution domain via linux personality

0e5559b

carry opencontainers#3126 Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 25, 2023

linux: Support setting execution domain via linux personality

7fe4246

carry opencontainers#3126 Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 25, 2023

linux: Support setting execution domain via linux personality

ba6f8aa

carry opencontainers#3126 Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 26, 2023

linux: Support setting execution domain via linux personality

29705b7

carry opencontainers#3126 Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 26, 2023

linux: Support setting execution domain via linux personality

6220747

carry opencontainers#3126 Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

Zheaoli added a commit to Zheaoli/runc that referenced this pull request Oct 27, 2023

linux: Support setting execution domain via linux personality

98511bb

carry opencontainers#3126 Co-authored-by: Aditya R <arajan@redhat.com> Signed-off-by: Zheao.Li <me@manjusaka.me>

lifubang added a commit that referenced this pull request Oct 28, 2023

Merge pull request #4082 from Zheaoli/manjusaka/support-personality

6aa4d8d

carry #3126: linux: Support setting execution domain via linux personality

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`linux`: Support setting execution domain via linux `personality`. #3126

`linux`: Support setting execution domain via linux `personality`. #3126

flouthoc commented Aug 3, 2021

flouthoc Aug 3, 2021

AkihiroSuda Aug 3, 2021

flouthoc Aug 3, 2021

kolyshkin commented Aug 3, 2021

kolyshkin commented Aug 3, 2021

flouthoc commented Aug 3, 2021 •

edited

Loading

kolyshkin commented Aug 3, 2021

kolyshkin commented Aug 3, 2021

flouthoc commented Aug 4, 2021

flouthoc commented Aug 5, 2021 •

edited

Loading

flouthoc commented Aug 6, 2021

kolyshkin left a comment

flouthoc commented Aug 11, 2021

flouthoc commented Aug 11, 2021

flouthoc commented Aug 18, 2021

kolyshkin Aug 31, 2021

flouthoc Aug 31, 2021

kolyshkin Aug 31, 2021

flouthoc Sep 1, 2021

cyphar Sep 9, 2021

flouthoc Sep 9, 2021

cyphar Sep 9, 2021

cyphar Sep 9, 2021

cyphar Sep 9, 2021

cyphar Sep 9, 2021

AkihiroSuda commented Feb 1, 2023

kolyshkin commented Sep 26, 2023

Zheaoli commented Sep 27, 2023

AkihiroSuda commented Sep 27, 2023

flouthoc commented Sep 27, 2023 •

edited

Loading

Zheaoli commented Sep 27, 2023

AkihiroSuda commented Oct 20, 2023

Zheaoli commented Oct 20, 2023

		// check libcontainer/configs/config_linux.go
		// check https://raw.githubusercontent.com/torvalds/linux/master/include/uapi/linux/personality.h

linux: Support setting execution domain via linux personality. #3126

linux: Support setting execution domain via linux personality. #3126

Conversation

flouthoc commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Aug 3, 2021

kolyshkin commented Aug 3, 2021

flouthoc commented Aug 3, 2021 • edited Loading

kolyshkin commented Aug 3, 2021

kolyshkin commented Aug 3, 2021

flouthoc commented Aug 4, 2021

flouthoc commented Aug 5, 2021 • edited Loading

flouthoc commented Aug 6, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

flouthoc commented Aug 11, 2021

flouthoc commented Aug 11, 2021

flouthoc commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Feb 1, 2023

kolyshkin commented Sep 26, 2023

Zheaoli commented Sep 27, 2023

AkihiroSuda commented Sep 27, 2023

flouthoc commented Sep 27, 2023 • edited Loading

Zheaoli commented Sep 27, 2023

AkihiroSuda commented Oct 20, 2023

Zheaoli commented Oct 20, 2023

`linux`: Support setting execution domain via linux `personality`. #3126

`linux`: Support setting execution domain via linux `personality`. #3126

flouthoc commented Aug 3, 2021 •

edited

Loading

flouthoc commented Aug 5, 2021 •

edited

Loading

flouthoc commented Sep 27, 2023 •

edited

Loading