cgroup: fix NULL device access and restore the no-cgroup invariant - #2206
Open
kolyshkin wants to merge 5 commits into
Open
cgroup: fix NULL device access and restore the no-cgroup invariant#2206kolyshkin wants to merge 5 commits into
kolyshkin wants to merge 5 commits into
Conversation
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
`access` is optional on `linux.resources.devices[]`, so libocispec leaves it NULL whenever the field is omitted -- the same case just fixed in is_rwm(). write_devices_resources_v1() passes it straight to snprintf() with a %s conversion, which is undefined behaviour: glibc substitutes the literal "(null)" and writes `c 1:3 (null)` into devices.allow/devices.deny, other implementations are free to crash. The cgroup v2 path already maps a NULL access to "" inside bpf_program_append_dev(), so the two backends disagreed about a device entry the OCI spec allows. Make v1 agree with v2. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Several helpers build a cgroup path with append_paths() from a path that may be empty, either because a container has no cgroup or because the caller passed NULL. append_paths() stops at the first NULL argument, so such a call silently yields the cgroup root instead of failing: - destroy_cgroup_path() would rmdir() the cgroup root, - setup_rt_runtime() would write cpu.rt_runtime_us of the root cgroup. get_cgroup_scope_path() has the opposite problem: it feeds its argument to xstrdup(), which returns NULL for NULL, and then to strchr(). None of this is reachable today, since a container without cgroups is recorded with an empty cgroup path and every affected caller happens to be gated on something else. That changes in the next commit, which restores NULL as the representation of "no cgroup". Guard the three helpers first so that no single commit leaves the tree exposed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
libcrun_cgroup_enter_disabled() sets `path` to NULL for a container that runs without cgroups, and helpers all over the tree take NULL to mean exactly that -- libcrun_cgroup_has_oom(), cgroup_killall_path(), libcrun_cgroup_pause_unpause_with_mode() and others explicitly test for it. update_cgroup_resources() relies on it too, and only reports "cannot set limits without cgroups" when `path` is NULL. libcrun_cgroup_make_status(), which rebuilds that struct from the on-disk status, broke the invariant: the status always carries a `cgroup-path` key -- reading fails outright when it is missing -- and it holds an empty string for a container without cgroups, so xstrdup() turned NULL into "". Note how the very next statement in that function already uses is_empty_string() to derive the cgroup manager. As a result `crun update` on a container started with --cgroup-manager=disabled skipped the check and tried to open cgroup files under the root: $ crun --cgroup-manager=disabled update --memory 536870912 CT open `memory.max` for writing: No such file or directory instead of the intended "cannot set limits without cgroups". It also left the NULL-access crash fixed two commits ago unreachable. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Both tests drive `crun update` against a container started with
--cgroup-manager=disabled, and both fail on the tree before this series:
- update-without-cgroup-memory reports the raw
"open `memory.max` for writing" instead of the intended refusal,
- update-without-cgroup-device-no-access dies with SIGSEGV once the
no-cgroup branch becomes reachable again.
The helper checks for a negative return code separately, so that a crash
is reported as such rather than as a mismatched error message.
Unlike the rest of test_update.py these two need no cgroup and therefore
no root, so they run rootless as well. The NULL access fix in the cgroup
v1 writer is left uncovered: no test in the tree requires cgroup v1 and
CI does not set it up, so such a test would never execute.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative to #2203, which is included here as the first commit (authorship preserved).
#2203 by itself fixes a crash in a branch that cannot currently be reached:
libcrun_cgroup_make_status()stores an empty string where the rest of the tree expects NULL for "no cgroup", soupdate_cgroup_resources()'spath == NULLguard never fires. The same defect makescrun updateon a container started with--cgroup-manager=disabledreportopen \memory.max` for writing: No such file or directoryinstead ofcannot set limits without cgroups`.The series fixes the latent NULL dereferences first and flips the invariant last, so no single commit leaves a reachable crash:
is_rwm()NULL access (cgroup-resources: treat NULL device access as empty in is_rwm #2203, by @SAY-5)snprintf("%s", NULL)in the cgroup v1 device writer, also reported in cgroup-resources:NULLdereference ofaccessfield #2195destroy_cgroup_path(),get_cgroup_scope_path()andsetup_rt_runtime()libcrun_cgroup_make_status()-- the root causeCloses #2195
cc @eriksjolund