Skip to content
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.

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

seccomp: fix go-specs for errnoRet #1042

Merged
merged 1 commit into from
May 19, 2020

Conversation

giuseppe
Copy link
Member

commit 3bfcde2 introduced errnoRet
for seccomp syscalls but the Go specs were not implemented correctly.

Signed-off-by: Giuseppe Scrivano [email protected]

commit 3bfcde2 introduced errnoRet
for seccomp syscalls but the Go specs were not implemented correctly.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@rhatdan
Copy link
Contributor

rhatdan commented May 12, 2020

@vbatts PTAL

@giuseppe
Copy link
Member Author

giuseppe commented May 12, 2020

@mrunalp PTAL

@@ -669,7 +669,7 @@ type LinuxSeccompArg struct {
type LinuxSyscall struct {
Names []string `json:"names"`
Action LinuxSeccompAction `json:"action"`
ErrnoRet uint `json:"errno"`
ErrnoRet *uint `json:"errnoRet,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering; Was the pointer needed? (wondering because IIRC, omitempty also omits 0, in which case the default for an uint would be a meaningful value (no override)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need the pointer here as specifying ErrnoRet = 0 has a different meaning (report success), while the default with ErrnoRet unspecified is EPERM.

I've done a test for omitempty and it seems pointers are omitted only if they are nil: https://play.golang.org/p/wnzK9EQNCOS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't ErrnoRet = 0 conflict with the purpose of LinuxSeccompAction ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the combination SCMP_ACT_ERRNO and ErrnoRet = 0 is useful and it is different than SCMP_ACT_ALLOW, since the syscall is not performed but it is reported as successful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha, yes, that makes sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's critical that it be clear whether errnoRet was unset (-EPERM) or 0 (a "success" but the syscall isn't run). As an aside, this trick is used for the relatively-new seccomp syscall emulation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed it would only be used with > 0 values (0 meaning; don't change the default); thx!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, it's definitely not the intended use for seccomp. ;)

@mrunalp
Copy link
Contributor

mrunalp commented May 12, 2020

LGTM

Approved with PullApprove

@cyphar
Copy link
Member

cyphar commented May 12, 2020

LGTM.

@giuseppe
Copy link
Member Author

@tianon
Copy link
Member

tianon commented May 19, 2020

LGTM

Approved with PullApprove

@tianon tianon merged commit 0230443 into opencontainers:master May 19, 2020
@AkihiroSuda AkihiroSuda mentioned this pull request Jan 24, 2023
@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Feb 1, 2023
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 26, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants