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

RFE: add RISC-V 64-bit support #197

Closed
wants to merge 1 commit into from
Closed

RFE: add RISC-V 64-bit support #197

wants to merge 1 commit into from

Conversation

andreas-schwab
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Jan 7, 2020

Coverage Status

Coverage decreased (-0.05%) to 89.763% when pulling ddbc2aa on andreas-schwab:master into ed3b35c on seccomp:master.

@pcmoore pcmoore changed the title arch: Add RISC-V 64-bit support RFE: add RISC-V 64-bit support Jan 8, 2020
@pcmoore pcmoore added this to the v2.5 milestone Jan 8, 2020
@pcmoore
Copy link
Member

pcmoore commented Jan 8, 2020

@drakenclimber I'm thinking this is worth adding to the v2.5 milestone, assuming everything is working and look okay. Thoughts?

@drakenclimber
Copy link
Member

@drakenclimber I'm thinking this is worth adding to the v2.5 milestone, assuming everything is working and look okay. Thoughts?

Yes, I agree. This would be a really good addition for v2.5

@carlosedp
Copy link

Hi folks, any news on this? Is it scheduled to be included on next version?
Thanks!

@drakenclimber
Copy link
Member

Hi folks, any news on this? Is it scheduled to be included on next version?
Thanks!

I'm still planning on trying to get it into v2.5. (Not sure yet on v2.5's release date yet, but pressure is growing to get it out.)

I'll try to review this code this week. Thanks!

Copy link
Member

@drakenclimber drakenclimber left a comment

Choose a reason for hiding this comment

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

The changes look reasonable to me. I had a few nitpick comments that would be good to address but nothing major.

I also ran the changes through arch-syscall-validate and the RISC-V stuff all passed.

Acked-by: Tom Hromatka <[email protected]>

include/seccomp.h.in Outdated Show resolved Hide resolved
src/arch-riscv64-syscalls.c Show resolved Hide resolved
src/arch-riscv64.c Show resolved Hide resolved
src/arch-riscv64.h Show resolved Hide resolved
tools/util.h Outdated Show resolved Hide resolved
Signed-off-by: Andreas Schwab <[email protected]>
#ifndef EM_RISCV
#define EM_RISCV 243
#endif /* EM_RISCV */
#define AUDIT_ARCH_RISCV64 (EM_RISCV|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
Copy link
Member

Choose a reason for hiding this comment

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

In keeping with the other tokens in seccomp.h, I would have expected AUDIT_ARCH_RISCV64 to be defined before it was used. However, if that is the only nit in this review we can move that while merging.

@pcmoore
Copy link
Member

pcmoore commented Feb 23, 2020

Merged into master via 5432e15, thanks everyone!

@andreas-schwab please re-test with master just to make sure everything is still working (neither @drakenclimber or myself have RISC-V machines), and please make an attempt to test the libseccomp releases to make sure we don't break anything in the future :)

@pcmoore pcmoore closed this Feb 23, 2020
@carlosedp
Copy link

Amazing, thanks everyone!
Cc. @davidlt

@pcmoore
Copy link
Member

pcmoore commented Feb 24, 2020

@carlosedp if you've got a RISC-V system handy, we would appreciate some extra testing help :)

@pcmoore pcmoore mentioned this pull request Feb 24, 2020
@andreas-schwab
Copy link
Author

https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:Factory/libseccomp/r/riscv64

@carlosedp
Copy link

Hey @pcmoore, I've built and tested libseccomp on my RISC-V board. All good:

...
Regression Test Summary
 tests run: 5229
 tests skipped: 114
 tests passed: 5229
 tests failed: 0
 tests errored: 0

@pcmoore
Copy link
Member

pcmoore commented Feb 24, 2020

@andreas-schwab I see a single failure in your test run:

[ 5388s] Test 52-basic-load%%001-00001 result:   FAILURE 52-basic-load rc=22

... is this maybe due to the build/CI tool either not allowing seccomp policies to be loaded, or simply running a kernel that doesn't support seccomp-bpf?

@andreas-schwab
Copy link
Author

What is needed to support seccomp-bpf? The same error happens on armv7l, and that runs natively. There are no restrictions on what a kernel can do in OBS.

@pcmoore
Copy link
Member

pcmoore commented Feb 25, 2020

At the very least the kernel needs to be build with support for CONFIG_SECCOMP_FILTER, which is dependent upon HAVE_ARCH_SECCOMP_FILTER that is defined in the arch/ABI specific kernel code.

Are you building your own kernels @andreas-schwab or are you using distro-kernels?

@andreas-schwab
Copy link
Author

This is a normal distro build, as you can see from the logfile, and CONFIG_SECCOMP_FILTER is enabled in all kernels.

@pcmoore
Copy link
Member

pcmoore commented Feb 25, 2020

This is a normal distro build, as you can see from the logfile, and CONFIG_SECCOMP_FILTER is enabled in all kernels.

Hmm, any chance you can help us debug why the load test was failing?

@pcmoore
Copy link
Member

pcmoore commented Feb 25, 2020

I'm going to reopen this issue until we can resolve this last problem.

Since @carlosedp got a clean run on a RISC-V board I'm guessing there is just some problem with the OBS build and not a larger problem with libseccomp on RISC-V.

@andreas-schwab
Copy link
Author

The riscv64 failure is due to qemu-linux-user rejecting to emulate seccomp.

@andreas-schwab
Copy link
Author

For the armv7l failure I see this:

prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) = 0
seccomp(SECCOMP_SET_MODE_STRICT, 1, NULL) = -1 EINVAL (Invalid argument)
seccomp(SECCOMP_SET_MODE_FILTER, 0, {len=4, filter=0x50e400}) = -1 EINVAL (Invalid argument)

@pcmoore
Copy link
Member

pcmoore commented Feb 25, 2020

Looks like I can't reopen this issue, bummer.

@pcmoore
Copy link
Member

pcmoore commented Feb 25, 2020

The riscv64 failure is due to qemu-linux-user rejecting to emulate seccomp.

Okay, I think we can ignore that failure then, thank you.

For the armv7l failure I see this:

prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) = 0
seccomp(SECCOMP_SET_MODE_STRICT, 1, NULL) = -1 EINVAL (Invalid argument)
seccomp(SECCOMP_SET_MODE_FILTER, 0, {len=4, filter=0x50e400}) = -1 EINVAL (Invalid argument)

The fist call prctl(PR_SET_NO_NEW_PRIVS, ...) sets NO_NEW_PRIVS, which is expected (anc configurable via a filter attribute. Regardless, it succeeds so there is nothing to worry about there.

The seccomp(SECCOMP_SET_MODE_STRICT, 1, NULL) is also expected as it is a quick test to see if the system supports the seccomp syscall, see the sys_chk_seccomp_syscall() function for more information. In this case it appears to return EINVAL which indicates that the system does support the seccomp() syscall; this is good and expected.

However, the final syscall, seccomp(SECCOMP_SET_MODE_FILTER, 0, {len=4, filter=0x50e400}) is a bit confusing as it returns EINVAL indicating an error. The len=4 tells us that the BPF filter contains four instructions, which I would expect (load ABI token, compare ABI token and jump to good/bad ABI, return ALLOW, return KILL). Are you 100% that this kernel is build with CONFIG_SECCOMP_FILTER?

@andreas-schwab
Copy link
Author

No, it doesn't, because it has OABI_COMPAT enabled.

@pcmoore
Copy link
Member

pcmoore commented Feb 25, 2020

Ah, ha! Mystery solved :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants