util: Fix GUIX build with syscall sandbox by laanwj · Pull Request #23178 · bitcoin/bitcoin

laanwj · 2021-10-04T22:11:50Z

Looks like we've broke the GUIX build in #20487. This attempts to fix it:

Define __NR_statx __NR_getrandom __NR_membarrier as some kernel headers lack them, and it's important to have the same profile independent on what kernel is used for building.
Define SECCOMP_RET_KILL_PROCESS as it isn't defined in the headers.

laanwj · 2021-10-04T22:21:22Z

Thinking of it, I'm not sure this is the right solution. statx is not directly used by us, but by libc. What if the resulting binary is run against a more recent version of libc (remember, we link libc dynamically), it will use the system call but not be allowed to, so fail?

Might be better to do

#ifndef __NR_statx
#define __NR_statx 332
#endif

…instead
And the same for __NR_getrandom and __NR_membarrier.

Edit: pushed this new solution

Define `SECCOMP_RET_KILL_PROCESS` as it isn't defined in the headers, as is the case for the GUIX build on this platform.

…d filesystem syscalls 44d77d2 sandbox: add copy_file_range to allowed filesystem syscalls (fanquake) ee08741 sandbox: add newfstatat to allowed filesystem syscalls (fanquake) Pull request description: Similar to #23178, this is a follow up to #20487, which has broken running the unit tests for some developers. Fix this by adding `newfstatat` to the list of allowed filesystem related calls. ACKs for top commit: achow101: ACK 44d77d2 laanwj: Code review ACK 44d77d2 practicalswift: cr ACK 44d77d2 Tree-SHA512: ce9d1b441ebf25bd2cf290566e05864223c1418dab315c962e1094ad877db5dd9fcab94ab98a46da8b712a8f5f46675d62ca3349215d8df46ec5b3c4d72dbaa6

practicalswift

cr ACK 1685d1221e7e605ff073df94e223420691afa079

This might be helpful for fellow reviewers:

$ grep -E '(statx|getrandom|membarrier)$' linux/arch/x86/entry/syscalls/syscall_64.tbl
318     common  getrandom               sys_getrandom
324     common  membarrier              sys_membarrier
332     common  statx                   sys_statx
$ grep SECCOMP_RET_KILL_PROCESS linux/include/uapi/linux/seccomp.h
#define SECCOMP_RET_KILL_PROCESS 0x80000000U /* kill the process */

maflcko · 2021-10-05T10:09:35Z

Can remove this line now?

https://github.com/bitcoin/bitcoin/blob/1685d1221e7e605ff073df94e223420691afa079/src/util/syscall_sandbox.cpp#L179

practicalswift · 2021-10-05T10:20:15Z

@MarcoFalke Yes, I think these two no longer need to be conditional:

#if defined(__NR_getrandom)
    {__NR_getrandom, "getrandom"},
#endif // defined(__NR_getrandom)
…
#if defined(__NR_membarrier)
    {__NR_membarrier, "membarrier"},
#endif // defined(__NR_membarrier)

laanwj · 2021-10-05T12:02:18Z

Can remove this line now?

I'm still not decided what I want to do with that table, but i i's supposed to be more or less platform-independent (see also discussion here: #20487 (comment) ). I left the conditional like this for platforms that really don't have the getrandom/membarrier call.

Define the following syscall numbers for x86_64, so that the profile will be the same no matter what kernel is built against, including kernels that don't have `__NR_statx`: ```c++ #define __NR_statx 332 #define __NR_getrandom 318 #define __NR_membarrier 324 ```

laanwj · 2021-10-05T12:43:09Z

Anyhow, removed them and force-pushed. I agree it's also somewhat confusing and they can always be added again.

practicalswift · 2021-10-05T12:56:12Z

cr ACK 2d02799

Thanks for quickly resolving this!

practicalswift · 2021-10-05T14:15:42Z

FWIW this is how minijail (a sandboxing and containment tool used in Chrome OS and Android) handles this:

/* Ideally minijail is compiled against a modern libc, which has modern copies
 * of Linux uapi for ioctls, and unistd.h for syscalls. However, sometimes this
 * isn't possible - such as when building with the Android host toolchain - so
 * locally define the system calls in use in active seccomp policy files.
 * This UAPI is taken from sanitized bionic headers.
 */

#ifndef __NR_copy_file_range
#ifdef __x86_64__
#define __NR_copy_file_range 326
#elif __i386__
#define __NR_copy_file_range 377
#elif __arm64__
#define __NR_copy_file_range 285
#endif
#endif /* __NR_copy_file_range */
…

https://github.com/google/minijail/blob/6aa0392f3a948c09eb8305650f5d3067e94b25af/gen_syscalls-inl.h#L8-L23

laanwj · 2021-10-05T14:24:35Z

Thanks—we could do the same when we support sandboxing on multiple platforms.

… allowed filesystem syscalls 44d77d2 sandbox: add copy_file_range to allowed filesystem syscalls (fanquake) ee08741 sandbox: add newfstatat to allowed filesystem syscalls (fanquake) Pull request description: Similar to bitcoin#23178, this is a follow up to bitcoin#20487, which has broken running the unit tests for some developers. Fix this by adding `newfstatat` to the list of allowed filesystem related calls. ACKs for top commit: achow101: ACK 44d77d2 laanwj: Code review ACK 44d77d2 practicalswift: cr ACK 44d77d2 Tree-SHA512: ce9d1b441ebf25bd2cf290566e05864223c1418dab315c962e1094ad877db5dd9fcab94ab98a46da8b712a8f5f46675d62ca3349215d8df46ec5b3c4d72dbaa6

2d02799 util: Make sure syscall numbers used in profile are defined (W. J. van der Laan) 8289d19 util: Define SECCOMP_RET_KILL_PROCESS if not provided by the headers (W. J. van der Laan) Pull request description: Looks like we've broke the GUIX build in bitcoin#20487. This attempts to fix it: - Define `__NR_statx` `__NR_getrandom` `__NR_membarrier` as some kernel headers lack them, and it's important to have the same profile independent on what kernel is used for building. - Define `SECCOMP_RET_KILL_PROCESS` as it isn't defined in the headers. ACKs for top commit: practicalswift: cr ACK 2d02799 Tree-SHA512: c264c66f90af76bf364150e44d0a31876c2ef99f05777fcdd098a23f1e80efef43028f54bf9b3dad016110056d303320ed9741b0cb4c6266175fa9d5589b4277

DrahtBot · 2021-10-06T07:59:05Z

Guix builds

File	commit `816e15e` (master)	commit 0afd60bcfb0a7f853210eb2c3af94283ba182b8d (master and this pull)
*.tar.gz	`1350388139b5ea3d...`	`5ddf35dfc76a4aa8...`
guix_build.log	`d856056de4d6bf62...`	`e61e6e6cb6397634...`
SHA256SUMS.part		`d04222fdf9f985fb...`
*-aarch64-linux-gnu-debug.tar.gz		`e6dc2147b545c6f2...`
*-aarch64-linux-gnu.tar.gz		`3c0982848ecc3b2a...`
*-arm-linux-gnueabihf-debug.tar.gz		`00746f713efec4cb...`
*-arm-linux-gnueabihf.tar.gz		`d20f0d3455324cd3...`
*-osx-unsigned.dmg		`beb1174d759bf397...`
*-osx-unsigned.tar.gz		`fc509bd7c1dc487c...`
*-osx64.tar.gz		`531609737c6de7b0...`
*-powerpc64-linux-gnu-debug.tar.gz		`0f9acb5de2c66a4c...`
*-powerpc64-linux-gnu.tar.gz		`bc25c8fd07d634e1...`
*-powerpc64le-linux-gnu-debug.tar.gz		`c1264831b1aabf6c...`
*-powerpc64le-linux-gnu.tar.gz		`32becb071c8b7059...`
*-riscv64-linux-gnu-debug.tar.gz		`4289c2afd7fb0692...`
*-riscv64-linux-gnu.tar.gz		`f62a44650044edcf...`
*-win-unsigned.tar.gz		`8ab05ff22cc2a254...`
*-win64-debug.zip		`48cee67be9779712...`
*-win64-setup-unsigned.exe		`d15e7c917e6db189...`
*-win64.zip		`7acd902e319924a5...`
*-x86_64-linux-gnu-debug.tar.gz		`a4b29211c64b4f10...`
*-x86_64-linux-gnu.tar.gz		`65163bd030110994...`
guix_build.log.diff		`ed83cc9d8254dede...`

laanwj added Build system Utils/log/libs labels Oct 4, 2021

laanwj requested review from dongcarl and practicalswift October 4, 2021 22:11

fanquake mentioned this pull request Oct 5, 2021

sandbox: add newfstatat & copy_file_range to allowed filesystem syscalls #23179

Merged

util: Define SECCOMP_RET_KILL_PROCESS if not provided by the headers

8289d19

Define `SECCOMP_RET_KILL_PROCESS` as it isn't defined in the headers, as is the case for the GUIX build on this platform.

laanwj force-pushed the 2021-10-guix-linux-sandbox branch from 03518b6 to 5f626ac Compare October 5, 2021 06:17

maflcko added the DrahtBot Guix build requested label Oct 5, 2021

laanwj force-pushed the 2021-10-guix-linux-sandbox branch from 5f626ac to 1685d12 Compare October 5, 2021 06:59

practicalswift approved these changes Oct 5, 2021

View reviewed changes

laanwj force-pushed the 2021-10-guix-linux-sandbox branch from 1685d12 to 2d02799 Compare October 5, 2021 12:42

laanwj merged commit 89b9107 into bitcoin:master Oct 5, 2021

ryanofsky mentioned this pull request Oct 5, 2021

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487

Merged

DrahtBot removed the DrahtBot Guix build requested label Oct 6, 2021

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: Fix GUIX build with syscall sandbox#23178

util: Fix GUIX build with syscall sandbox#23178
laanwj merged 2 commits intobitcoin:masterfrom
laanwj:2021-10-guix-linux-sandbox

laanwj commented Oct 4, 2021 •

edited

Loading

Uh oh!

laanwj commented Oct 4, 2021 •

edited

Loading

Uh oh!

practicalswift left a comment

Uh oh!

maflcko commented Oct 5, 2021

Uh oh!

practicalswift commented Oct 5, 2021

Uh oh!

laanwj commented Oct 5, 2021 •

edited

Loading

Uh oh!

laanwj commented Oct 5, 2021

Uh oh!

practicalswift commented Oct 5, 2021

Uh oh!

practicalswift commented Oct 5, 2021

Uh oh!

laanwj commented Oct 5, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

laanwj commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 5, 2021

Uh oh!

practicalswift commented Oct 5, 2021

Uh oh!

laanwj commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 5, 2021

Uh oh!

practicalswift commented Oct 5, 2021

Uh oh!

practicalswift commented Oct 5, 2021

Uh oh!

laanwj commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 6, 2021

Guix builds

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laanwj commented Oct 4, 2021 •

edited

Loading

laanwj commented Oct 4, 2021 •

edited

Loading

laanwj commented Oct 5, 2021 •

edited

Loading

laanwj commented Oct 5, 2021 •

edited

Loading