Skip to content

Conversation

@nalind
Copy link
Member

@nalind nalind commented Jan 22, 2020

When a seccomp rule includes multiple equality checks for the same argument for a syscall, they can never ALL be satisfied. Because that's how they're supposed to be treated, libseccomp returns an error when we try to add them as part of the same conditional rule. Try to detect this exact case, and if we detect it, treat each condition as its own rule.

When a seccomp rule includes multiple equality checks for the same
argument for a syscall, they can never ALL be satisfied.  Because that's
how they're supposed to be treated, libseccomp returns an error when we
try to add them as part of the same conditional rule.  Try to detect
this exact case, and if we detect it, treat each condition as its own
rule.

Signed-off-by: Nalin Dahyabhai <[email protected]>
// were OR'd, when in fact they're ordinarily
// supposed to be AND'd. Break them up into
// different rules to get that OR effect.
if len(rule.Args) > 1 && opsAreAllEquality && err.Error() == "two checks on same syscall argument" {
Copy link
Member

Choose a reason for hiding this comment

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

Where's the string from? It always worries me to check for an error by looking at the string output rather than a particular error type.

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 share your concern. This is an error object that github.com/seccomp/libseccomp-golang creates every time it returns the error, so we can't just compare the error itself for equality with a known value. If there's a better method, I'm happy to switch to it.

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM
@rhatdan @vrothberg PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

I have a hard time reviewing this. Is there any chance we can generalize the problem and get it into libseccomp-golang?

@rhatdan
Copy link
Member

rhatdan commented Jan 28, 2020

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit f4f2c05 has been approved by rhatdan

@rh-atomic-bot
Copy link
Collaborator

⌛ Testing commit f4f2c05 with merge 1033abc...

@rh-atomic-bot
Copy link
Collaborator

☀️ Test successful - status-papr, status-travis
Approved by: rhatdan
Pushing 1033abc to master...

@vrothberg
Copy link
Member

I have a hard time reviewing this. Is there any chance we can generalize the problem and get it into libseccomp-golang?

Why was it merged without a reply to my concerns above?

@rhatdan
Copy link
Member

rhatdan commented Jan 29, 2020

I saw this as a separate PR, not necessarily related to Buildah. I am also wondering if the chroot code is specific to Buildah, and not sure if this is useful to other projects.

@nalind
Copy link
Member Author

nalind commented Feb 5, 2020

FWIW, we're doing this here instead of the seccomp bindings to mirror part of how runc does things in opencontainers/runc#1616.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants