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

fix event.c for seccomp and ptrace #115

Closed
wants to merge 1 commit into from
Closed

fix event.c for seccomp and ptrace #115

wants to merge 1 commit into from

Conversation

jorge-lip
Copy link
Contributor

No description provided.

@jopasserat
Copy link
Member

Hey @jorge-lip thanks for the contribution!
However I'm afraid it doesn't work...

I've built and tested your branch against a basic ls

proot -v 3 ls

And I get the exact same output as with your branch, whereas I should see the log message from the seccomp mode activation:

proot info: ptrace acceleration (seccomp mode 2) enabled

You can go and try for yourself with the two static builds that I made available with and without seccomp enabled: https://github.com/proot-me/proot-static-build/releases/tag/v5.1.1

@jopasserat
Copy link
Member

Just to be clear, your solution is one of the walls @romainreuillon I and hit while trying to solve the issue: it seems to me that your fix basically disables the seccomp mode for the rest of the execution.

@jorge-lip
Copy link
Contributor Author

jorge-lip commented Feb 14, 2017 via email

@jorge-lip
Copy link
Contributor Author

jorge-lip commented Feb 14, 2017 via email

@alkino
Copy link
Contributor

alkino commented Feb 15, 2017

Thanks for the patch @jorge-lip

It seems me really weird that you fall-through from SIGTRAP to SECCOMP in the switch.

@jopasserat
Copy link
Member

@jorge-lip yes the static build is a bit rough as of now: you've got to place a tarball of your source tree under the packages directory. I'll have another look as I can indeed see the seccomp mode enabled with the binary from your owncloud

@jopasserat
Copy link
Member

I messed up my static build, I now confirm that it works on my system too. Will evaluate the performance on a real-world application that was facing a huge overhead in the PROOT_NO_SECCOMP mode.

@twhitehead
Copy link

twhitehead commented Mar 16, 2017

Not sure about any performance issues, but this seems to be the only branch I've found that works in an lxc container with seccomp enabled under Fedora 23 running kernel 4.8.14.

@vincenthage
Copy link
Contributor

Hi,

Can you update this PR (either with a small dummy commit, or by closing/opening it) so that the Travis CI can run the tests and verify that everything checks out?

Thanks

@vincenthage vincenthage closed this Sep 7, 2017
@vincenthage vincenthage reopened this Sep 7, 2017
@vincenthage
Copy link
Contributor

All tests pass, PR is ready to merge.

@vincenthage
Copy link
Contributor

vincenthage commented Sep 7, 2017

Though to be sure, can we get a check from everyone that this PR effectively fixes the SECCOMP issue please? It should at least be checked on old (2.6.32) and recent kernels (4.8, 4.9, 4.11).

@vincenthage
Copy link
Contributor

It looks like the Travis tests return a false positive, the tests aren't actually run.

@tsjk
Copy link

tsjk commented Sep 26, 2017

I'm confused by the state of this. Downloading and running the referenced binary and all is joy, but building (either normally, or statically using the Docker build system) the latest master (fdc253b) produces a binary that segfaults with SECCOMP enabled. What is the actual state of the art here?

@jopasserat
Copy link
Member

Hey @tsjk, the master branch doesn't this fix yet as it doesn't pass all the tests in Travis.

Ideally we'd love to have a specific test for this fix, but not sure what would be representative. Any idea @jorge-lip ?

Also, @mirage335 reported the fix was not working on his specific configuration #106 (comment). Not reproduced since then.

We're already using the fix in @openmole without any problem so far.
We're building the static binaries via the Docker build and manually merge this branch in master along with @vincenthage 's Network cooperation mode

Really looking forward to have all these nice fixes/features merged in master and release the binaries!

@jopasserat
Copy link
Member

On a separate note, do you think you could solve the merge conflict @jorge-lip?
It's nothing big, just a now unused variable since I merged next in master and an indentation glitch (https://github.com/proot-me/PRoot/pull/115/conflicts).

@tsjk
Copy link

tsjk commented Sep 26, 2017

Oh, I see. Thanks for your answer. I'm looking forward to the upcoming release. I'm using this with junest. In the meanwhile I've created a gist for myself that seems to produce a working and updated (x86_64) binary; https://gist.github.com/tsjk/10a9c64eae6e41ecd262a043b1a14907. You probably have something similar already.

@loveshack
Copy link

In case the data are useful, I tried the patch in a Fedora rawhide VM (linux "4.14.0-0.rc6.git2.1") and got the following failures -- more than the travis report:

CHECK test-2db65cd2 FAILED
CHECK test-d92b57ca FAILED
CHECK test-cdd39012 FAILED
CHECK test-0228fbe7 FAILED
CHECK test-c47aeb7d FAILED
CHECK test-a3e68988 FAILED

@romainreuillon
Copy link

Any hope to see this PR merged? What would it take to fix the test?

I would love to see a proot version with the portmap and the "kill on exit" support integrated in udocker.

@jorge-lip
Copy link
Contributor Author

Hi in udocker I'm now using a different patch, there were still situations
were I was getting issues when spawning processes. I think I will make
a new PR with this patch against the master branch. I anyone wants to
try it udocker ships with a patched proot and the patch is in udocker
github.

@jorge-lip
Copy link
Contributor Author

Are the patches for portmap and the "kill on exit" support already integrated in the proot
master branch ?

@romainreuillon
Copy link

Yes, they both have been merged.

@jopasserat
Copy link
Member

that's great news @jorge-lip looking forward to that!

@jorge-lip jorge-lip closed this Nov 7, 2017
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.

8 participants