-
-
Notifications
You must be signed in to change notification settings - Fork 205
docker*.sh scripts: add kvm support and missing authentication so that .docker_*.sh make BOARD=qemu* run launches kvm through gtk window as expected #2036
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the Docker wrapper scripts to enable KVM acceleration and X11 GUI support for QEMU board testing. Previously tested only under QubesOS, these changes allow the scripts to work with standard Linux environments running KVM-accelerated QEMU with GTK windows.
Key changes:
- Added
/dev/kvmdevice passthrough to enable KVM hardware acceleration in containers - Added X11 socket and authentication mounting to support GUI applications (GTK windows)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| docker_repro.sh | Added KVM device passthrough and X11 authentication mounts to enable hardware-accelerated QEMU with GUI support |
| docker_local_dev.sh | Added KVM device passthrough and X11 authentication mounts to enable hardware-accelerated QEMU with GUI support |
| docker_latest.sh | Added KVM device passthrough and X11 authentication mounts to enable hardware-accelerated QEMU with GUI support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t .docker_*.sh make BOARD=qemu* run launches kvm through gtk window as expected Refactored the 3 scripts with better checking and automatization for docker_local_dev.sh TODO: cleanup of README.md since most is now documented as code, outside of installing docker-ce and nix Signed-off-by: Thierry Laurion <insurgo@riseup.net>
c127e5a to
510b43e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Add Xauthority if it exists | ||
| if [ -f "${HOME}/.Xauthority" ]; then | ||
| opts="${opts} -v ${HOME}/.Xauthority:/root/.Xauthority:ro" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Xauthority file is mounted read-only into the container at /root/.Xauthority. This allows any process in the container to use the host's X11 authentication credentials. While this is necessary for X11 display forwarding, be aware this grants the container access to display on the host's X server. Consider documenting this security implication, especially since the container runs as root.
| # Build Docker options and execute | ||
| DOCKER_RUN_OPTS=$(build_docker_opts) | ||
|
|
||
| # shellcheck disable=SC2086 |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shellcheck disable directive for SC2086 (word splitting) is used without explanation. Consider adding a comment explaining why word splitting is intentionally needed here (to expand the space-separated Docker options).
| # shellcheck disable=SC2086 | |
| # shellcheck disable=SC2086 # Intentional word splitting: DOCKER_RUN_OPTS contains space-separated Docker options that must be expanded as separate arguments. |
| kill_usb_processes() { | ||
| # check if scdaemon or pcscd processes are using USB devices | ||
| if [ -d /dev/bus/usb ]; then | ||
| if sudo lsof /dev/bus/usb/00*/0* 2>/dev/null | awk 'NR>1 {print $2}' | xargs -r ps -p | grep -E 'scdaemon|pcscd' >/dev/null; then | ||
| echo "Killing GPG toolstack related processes using USB devices..." | ||
| sudo lsof /dev/bus/usb/00*/0* 2>/dev/null | awk 'NR>1 {print $2}' | xargs -r ps -p | grep -E 'scdaemon|pcscd' | awk '{print $1}' | xargs -r sudo kill -9 | ||
| fi | ||
| if [ ! -d "/dev/bus/usb" ]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| if sudo lsof /dev/bus/usb/00*/0* 2>/dev/null | \ | ||
| awk 'NR>1 {print $2}' | \ | ||
| xargs -r ps -p | \ | ||
| grep -E 'scdaemon|pcscd' >/dev/null 2>&1; then | ||
| echo "Killing GPG toolstack related processes using USB devices..." | ||
| sudo lsof /dev/bus/usb/00*/0* 2>/dev/null | \ | ||
| awk 'NR>1 {print $2}' | \ | ||
| xargs -r ps -p | \ | ||
| grep -E 'scdaemon|pcscd' | \ | ||
| awk '{print $1}' | \ | ||
| xargs -r sudo kill -9 | ||
| fi | ||
| } |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kill_usb_processes function is duplicated across docker_repro.sh, docker_latest.sh, and docker_local_dev.sh. Consider extracting this common function into a shared library file to reduce code duplication and make maintenance easier.
| echo "Using CircleCI Docker image: ${DOCKER_IMAGE}" | ||
|
|
||
| # Handle Ctrl-C gracefully | ||
| trap "echo 'Script interrupted. Exiting...'; exit 130" SIGINT |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SIGINT trap exits with code 130, which is the conventional exit code for a process terminated by SIGINT (128 + 2). However, since this is using 'exec' on line 141, the trap may not be triggered as expected when the Docker process receives SIGINT. The signal would go directly to the Docker process. Consider whether this trap is still useful given the use of 'exec'.
| trap "echo 'Script interrupted. Exiting...'; exit 130" SIGINT |
| echo "Using the latest Docker image: ${DOCKER_IMAGE}" | ||
|
|
||
| # Handle Ctrl-C gracefully | ||
| trap "echo 'Script interrupted. Exiting...'; exit 130" SIGINT |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SIGINT trap exits with code 130, which is the conventional exit code for a process terminated by SIGINT (128 + 2). However, since this is using 'exec' on line 127, the trap may not be triggered as expected when the Docker process receives SIGINT. The signal would go directly to the Docker process. Consider whether this trap is still useful given the use of 'exec'.
| trap "echo 'Script interrupted. Exiting...'; exit 130" SIGINT |
| EOF | ||
|
|
||
| # Handle Ctrl-C gracefully | ||
| trap "echo 'Script interrupted. Exiting...'; exit 130" SIGINT |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SIGINT trap exits with code 130, which is the conventional exit code for a process terminated by SIGINT (128 + 2). However, since this is using 'exec' on line 220, the trap may not be triggered as expected when the Docker process receives SIGINT. The signal would go directly to the Docker process. Consider whether this trap is still useful given the use of 'exec'.
| trap "echo 'Script interrupted. Exiting...'; exit 130" SIGINT |
|
|
||
| # Build Docker options and execute | ||
| DOCKER_RUN_OPTS=$(build_docker_opts) | ||
|
|
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shellcheck disable directive for SC2086 (word splitting) is used without explanation. Consider adding a comment explaining why word splitting is intentionally needed here (to expand the space-separated Docker options).
| # Intentionally allow word splitting to expand space-separated Docker options. |
|
|
||
| # Build Docker options and execute | ||
| DOCKER_RUN_OPTS=$(build_docker_opts) | ||
|
|
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shellcheck disable directive for SC2086 (word splitting) is used without explanation. Consider adding a comment explaining why word splitting is intentionally needed here (to expand the space-separated Docker options).
| # Intentionally allow word splitting to expand the space-separated Docker options. |
| # Extract Docker image version from CircleCI config | ||
| if [ ! -f "${CONFIG_FILE}" ]; then | ||
| echo "Error: ${CONFIG_FILE} not found" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| DOCKER_IMAGE=$(grep -oP '^\s*-?\s*image:\s*\K(tlaurion/heads-dev-env:[^\s]+)' "${CONFIG_FILE}" | head -n 1) | ||
|
|
||
| if [ -z "${DOCKER_IMAGE}" ]; then | ||
| echo "Error: Docker image not found in ${CONFIG_FILE}" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Using CircleCI Docker image: ${DOCKER_IMAGE}" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker_repro.sh derives DOCKER_IMAGE from .circleci/config.yml as tlaurion/heads-dev-env:[tag] and then runs it directly, meaning reproducible builds depend on a mutable, third-party Docker tag instead of an immutable image identifier. If the remote image tag is re-published or the Docker registry account is compromised, an attacker could silently change the build environment to inject backdoors into produced firmware or misuse attached USB tokens/TPM via the passed-through devices. To harden the build supply chain, pin this image to an immutable digest (e.g., @sha256:...), verify it in the repo/CI, or build the image from version-controlled sources locally instead of trusting a mutable tag.
| # CONSTANTS | ||
| # ============================================================================ | ||
|
|
||
| readonly DOCKER_IMAGE="tlaurion/heads-dev-env:latest" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker_latest.sh hard-codes DOCKER_IMAGE="tlaurion/heads-dev-env:latest", which causes Docker to pull and execute whatever image is currently published under that mutable latest tag. If the Docker Hub repository or tag is compromised, a malicious image could be served to developers, gaining access to host USB/KVM devices and build outputs when this script is used for Heads development or testing. To reduce supply chain risk, avoid latest and instead pin to a vetted, immutable image reference (for example a specific tag plus digest) or a locally built image under project control.
My bad: I always only tested qemu boards under QubesOS (which as of now still doesn't permit nested xen->kvm virtualization)
This change permits to run things under qemu+kvm
Tested
Note: TPM DUK is mitigation for any OSes not providing efifb in their initramfs, prior of specialized drm+gpu drivers being loaded (at LUKS passphrase prompt early in init/systemd).
ISO tested: