seccomp: add riscv64 mapping to seccomp_linux.go by gdams · Pull Request #48455 · moby/moby

gdams · 2024-09-09T20:18:11Z

fixes: seccomp not correctly detecting riscv64 architecture #48454
relates to 4c2f18f / seccomp: support riscv64 #43553

- What I did

I added support for the riscv64 architecture in Docker's seccomp profile by updating the architecture mappings. Specifically, I included riscv64 in the nativeToSeccomp and goToNative maps in Docker's seccomp profile handling code.

- How I did it

Updated the nativeToSeccomp map to include "riscv64": specs.ArchRISC_V64 for proper architecture mapping.
Updated the goToNative map to include "riscv64": "riscv64" for correct runtime architecture detection.
Ensured that Docker’s seccomp profile now correctly applies for RISC-V-based containers.

- How to verify it

Run a Docker container on a native riscv64 system.
Confirm that the riscv_flush_icache system call and other RISC-V-specific syscalls are correctly applied in the seccomp profile.
Ensure that no architecture mismatch errors or seccomp-related syscall restrictions occur for the RISC-V architecture.
Test the functionality of other architectures to ensure no regressions in behavior.

- Description for the changelog

Add support for RISC-V (riscv64) architecture in Docker's seccomp profile handling.

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: George Adams <[email protected]>

tianon

I tested this, although in a little bit of a roundabout way (the way I'm running into the issue on our official DOI build infrastructure, in fact 😄)

I took this patch and applied it as-is to the vendored copy of this file in BuildKit, then used that to docker buildx build https://github.com/adoptium/containers.git#07677395574f5d3462c3b6fdf5f6c4a0a350b683:21/jdk/ubuntu/noble

Before:

 > [3/5] RUN set -eux;     ARCH="$(dpkg --print-architecture)";     case "${ARCH}" in        amd64)          ESUM='51fb4d03a4429c39d397d3a03a779077159317616550e4e71624c9843083e7b9';          BINARY_URL='https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.4%2B7/OpenJDK21U-jdk_x64_linux_hotspot_21.0.4_7.tar.gz';          ;;        arm64)          ESUM='d768eecddd7a515711659e02caef8516b7b7177fa34880a56398fd9822593a79';          BINARY_URL='https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.4%2B7/OpenJDK21U-jdk_aarch64_linux_hotspot_21.0.4_7.tar.gz';          ;;        ppc64el)          ESUM='c208cd0fb90560644a90f928667d2f53bfe408c957a5e36206585ad874427761';          BINARY_URL='https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.4%2B7/OpenJDK21U-jdk_ppc64le_linux_hotspot_21.0.4_7.tar.gz';          ;;        riscv64)          ESUM='b04fd7f52d18268a935f1a7144dae802b25db600ec97156ddd46b3100cbd13da';          BINARY_URL='https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.4%2B7/OpenJDK21U-jdk_riscv64_linux_hotspot_21.0.4_7.tar.gz';          ;;        s390x)          ESUM='c900c8d64fab1e53274974fa4a4c736a5a3754485a5c56f4947281480773658a';          BINARY_URL='https://github.com/adoptium/temurin21-binaries/releases/download/jdk-21.0.4%2B7/OpenJDK21U-jdk_s390x_linux_hotspot_21.0.4_7.tar.gz';          ;;        *)          echo "Unsupported arch: ${ARCH}";          exit 1;          ;;     esac;     wget --progress=dot:giga -O /tmp/openjdk.tar.gz ${BINARY_URL};     echo "${ESUM} */tmp/openjdk.tar.gz" | sha256sum -c -;     mkdir -p "/opt/java/openjdk";     tar --extract         --file /tmp/openjdk.tar.gz         --directory "/opt/java/openjdk"         --strip-components 1         --no-same-owner     ;     rm -f /tmp/openjdk.tar.gz /opt/java/openjdk/lib/src.zip;     find "/opt/java/openjdk/lib" -name '*.so' -exec dirname '{}' ';' | sort -u > /etc/ld.so.conf.d/docker-openjdk.conf;     ldconfig;     java -Xshare:dump;:
/tmp/openjdk.tar.gz: OK
+ tar --extract --file /tmp/openjdk.tar.gz --directory /opt/java/openjdk --strip-components 1 --no-same-owner
 /tmp/openjdk.tar.gz /opt/java/openjdk/lib/src.zip
+ sort -u
+ find /opt/java/openjdk/lib -name *.so -exec dirname {} ;
+ ldconfig
+ java -Xshare:dump
[0.018s][error][os] Syscall: RISCV_FLUSH_ICACHE not available; error='Operation not permitted' (errno=EPERM)
Error occurred during initialization of VM
Unable to synchronize I-cache

After:

#10 exporting to docker image format
#10 sending tarball 140.4s done
#10 DONE 299.7s

tianon · 2024-09-09T21:16:14Z

(IMO it also makes sense to backport this, at the very least to v27)

thaJeztah · 2024-09-10T07:53:18Z

(IMO it also makes sense to backport this, at the very least to v27)

Yes, looks like the profile was updated in 4c2f18f / #43553, which was part of 23.0 (?)

Was also considering if we should look at adding more tests for these (https://github.com/moby/moby/tree/34182f6202507a1974e19b5140e2e36dc99892ff/contrib/syscall-test), although I guess that won't work unless we'd have this running int CI 🙈

thaJeztah

LGTM, thanks!

gdams · 2024-09-10T09:44:50Z

Was also considering if we should look at adding more tests for these (https://github.com/moby/moby/tree/34182f6202507a1974e19b5140e2e36dc99892ff/contrib/syscall-test), although I guess that won't work unless we'd have this running int CI 🙈

@thaJeztah I think I've got a working test case which I'll add in a follow-up PR if you'd like.

thaJeztah · 2024-09-10T10:25:42Z

Thanks! Yes, more tests are always appreciated. I should add a warning that (from a quick look), currently those syscall tests are run as part of the "integration-cli" suite, e.g.;

moby/integration-cli/docker_cli_run_unix_test.go

Lines 985 to 995 in 29f4a79

    
           // TestRunSeccompProfileDenyCloneUserns checks that 'docker run syscall-test' 
        
           // with a the default seccomp profile exits with operation not permitted. 
        
           func (s *DockerCLIRunSuite) TestRunSeccompProfileDenyCloneUserns(c *testing.T) { 
        
           	testRequires(c, testEnv.IsLocalDaemon, seccompEnabled) 
        
           	ensureSyscallTest(testutil.GetContext(c), c) 
        
           	icmd.RunCommand(dockerBinary, "run", "syscall-test", "userns-test", "id").Assert(c, icmd.Expected{ 
        
           		ExitCode: 1, 
        
           		Err:      "clone failed: Operation not permitted", 
        
           	}) 
        
           }

We froze that suite, because we want to move away from requiring the docker CLI itself to be used for tests defined in this repository, and where possible verify behavior without the CLI itself involved (tests involving the CLI should be part of the docker/cli repository's e2e suite). While that's not the case yet, we could possibly make an exception (and temporarily disable the check that prevents new tests are added to the integration-cli suite in CI).

Containerd also maintains a copy of the seccomp-profile; long term we should look if we can unify those (potentially moving the secomp profile to a separate module) and perhaps we should at some point look at porting those tests over as well.

Either way; that last part is definitely orthogonal.

Happy to help with the integration-cli part (and where needed to temporarily disable that check).

thaJeztah · 2024-09-10T10:27:18Z

Test-failiures should be unrelated (we have some known flakiness in master; see #48400.

Gave CI another kick to see if it goes green, but if it doesn't, I'll probably merge this one.

gdams · 2024-09-10T10:27:53Z

Test-failiures should be unrelated (we have some known flakiness in master; see #48400.

Gave CI another kick to see if it goes green, but if it doesn't, I'll probably merge this one.

That's great! do you need me to manually kick off the backports once this is merged or do you have some automated tooling?

thaJeztah · 2024-09-10T10:32:33Z

We don't have automated tooling (yet); the jury was still out on how to integrate automation but limiting permissions (i.e., we generally prefer automation to not have direct write access on the repository).

So cherry-picks are still manual; feel free to open backports for the release branch(es) though!

Some quick comments on that (we really need to write that down somewhere 🙈);

When cherry-picking, we prefer using the -x option, so that the cherry-picked commit has a reference to the original commit; i.e, something like

git cherry-pick -s -x <commit-sha>

(add the -S if you also (GPG)sign your commits)

And for backports, we want the PR description to have the branch in the title (e.g. [v27.x backport] ... or [v26.1 backport] ...

thaJeztah · 2024-09-10T10:33:43Z

LOL; and of course CI hit another flaky test (TestNetworkDBIslands this time); I'll bring this one in

gdams · 2024-09-10T10:42:28Z

@thaJeztah did you intentionally not add the process/cherry-pick/24.0 label or was that an accident?

thaJeztah · 2024-09-10T10:45:12Z

@gdams yes; the 24.0 branch is no longer maintained, so we generally skip it; see https://github.com/moby/moby/blob/master/project/BRANCHES-AND-TAGS.md#branches

(yet it's a bit confusing 🙈)

gdams · 2024-09-10T10:46:30Z

@gdams yes; the 24.0 branch is no longer maintained, so we generally skip it; see https://github.com/moby/moby/blob/master/project/BRANCHES-AND-TAGS.md#branches

(yet it's a bit confusing 🙈)

gotcha, wanted to double check! versions are always confusing ;) I've raised all the required backports then! Thanks for getting to this so quickly! I'm slightly amazed that it's taken 2 years for someone to pick this up but glad we've finally sorted it!

thaJeztah · 2024-09-10T10:46:44Z

🤔 We need to update that list to include 25.0, because AWS uses it (and it's planned to become the next LTS version for Mirantis IIUC). Lots of work to do improving our documentation around that 🙈

seccomp: add riscv64 mapping to seccomp_linux.go

1161b79

Signed-off-by: George Adams <[email protected]>

gdams force-pushed the seccomp branch from 1322b89 to 1161b79 Compare September 9, 2024 20:18

gdams mentioned this pull request Sep 9, 2024

eclipse-temurin: add support for riscv64 docker-library/official-images#17403

Merged

tianon approved these changes Sep 9, 2024

View reviewed changes

tianon added a commit to tianon/debian-moby that referenced this pull request Sep 9, 2024

Backport moby/moby#48455 for riscv64 seccomp

2ec3eaa

tianon mentioned this pull request Sep 9, 2024

Backport riscv64 seccomp profile fix to buildkit builds tianon/dockerfiles#800

Merged

thaJeztah added status/2-code-review area/security/seccomp kind/bugfix PR's that fix bugs process/cherry-pick/23.0 process/cherry-pick/25.0 process/cherry-pick/26.1 labels Sep 10, 2024

thaJeztah approved these changes Sep 10, 2024

View reviewed changes

thaJeztah added this to the 28.0.0 milestone Sep 10, 2024

thaJeztah merged commit e5a088d into moby:master Sep 10, 2024

gdams deleted the seccomp branch September 10, 2024 10:33

thaJeztah mentioned this pull request Sep 10, 2024

Enable riscv64 cross build #44735

Draft

thaJeztah removed process/cherry-pick/25.0 process/cherry-pick/26.1 labels Sep 10, 2024

thaJeztah added the process/cherry-picked label Sep 10, 2024

gdams mentioned this pull request Sep 10, 2024

[23.0 backport] seccomp: add riscv64 mapping to seccomp_linux.go #48466

Merged

thaJeztah removed the process/cherry-pick/23.0 label Sep 10, 2024

Conversation

gdams commented Sep 9, 2024 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon commented Sep 9, 2024

Uh oh!

thaJeztah commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

gdams commented Sep 10, 2024

Uh oh!

thaJeztah commented Sep 10, 2024

Uh oh!

thaJeztah commented Sep 10, 2024

Uh oh!

gdams commented Sep 10, 2024

Uh oh!

thaJeztah commented Sep 10, 2024

Uh oh!

thaJeztah commented Sep 10, 2024

Uh oh!

gdams commented Sep 10, 2024

Uh oh!

thaJeztah commented Sep 10, 2024

Uh oh!

gdams commented Sep 10, 2024

Uh oh!

thaJeztah commented Sep 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gdams commented Sep 9, 2024 •

edited by thaJeztah

Loading

thaJeztah commented Sep 10, 2024 •

edited

Loading