proc-cmdline: filter PID1 arguments on container #26887

yuwata · 2023-03-19T06:46:43Z

Otherwise, PID1 arguments e.g. "--deserialize 16" may be parsed unexpectedly by generators.

Fixes the issue reported at
#24452 (comment).

yuwata · 2023-03-19T06:46:55Z

cc @mrc0mmand

mrc0mmand

This seems to do the trick, I can't reproduce the original issue anymore. Thanks!

src/basic/proc-cmdline.c

yuwata · 2023-03-19T12:57:27Z

Updated quotation handling, and added tests. Unset the green label. PTAL.

@mrc0mmand Please test again. Thank you.

src/basic/proc-cmdline.c

mrc0mmand · 2023-03-19T15:03:55Z

Updated quotation handling, and added tests. Unset the green label. PTAL.

@mrc0mmand Please test again. Thank you.

Gave this a spin in both nspawn and QEMU with both coverage and sanitizers enabled (separately, of course), and everything still seems to be working as expected!

yuwata · 2023-03-19T15:17:51Z

@mrc0mmand Thank you!

keszybz

This fix is both too narrow and too wide in scope. Originally, filter_args() was added with in a specific context: when we're doing reexecution, so we know what args where we're taking the args from and where we are passing them to. But this patch generalizes the use of this function to pretty much all code that looks at the kernel command line, and I'm not sure if this is appropriate. I would expect at least a lot more discussion why we can and should do this so widely.

On the other hand, it is easy to show that this patch is too narrow. Generally, any code which looks at /proc/1/cmdline needs to handle all arguments that are supported by pid1, for example --log-level or --crash-chvt. This patch does not do this.

It is trivial to show that e.g. --log-level can be misinterpreted by systemd-debug-generator:

$ SYSTEMD_LOG_LEVEL=debug SYSTEMD_LOG_TARGET=console SYSTEMD_PROC_CMDLINE='--log-level 5 systemd.debug-shell' build/systemd-debug-generator /tmp/x/
SELinux enabled state cached to: disabled
$ find /tmp/x/
/tmp/x/
/tmp/x/graphical.target.wants
/tmp/x/graphical.target.wants/debug-shell.service

I think we should first think what proc_cmdline() should return in various contexts. E.g. we use it for ConditionKernelCommandLine=. Dunno, does it even make sense to try to not pass the full commandline here? To answer "yes", we would need to be able to provide some comprehensive and consistent story what is allowed to be visible to ConditionKernelCommandLine= and what is not. I doubt we can build such a story.

My instinct would be say that this is a bug in proc_cmdline_parse(). Looking at various parse_proc_cmdline_item() implementations, they generally only expect to be called on real /proc/cmdline items, and would be confused when called over long-option-style arguments and values. E.g. basic/log.c looks for a long debug and if that debug would be passed as --log-level debug it would be parsed unexpectedly there. So I think proc_cmdline_parse() should be adjusted to drop all long and short dash-options with arguments that are consumed by pid1. But other users of proc_cmdline() should get the full set and deal with it as appropriate.

I wrote some comments about the implementation before writing the big comment above… I'm leaving them in the review in case they would be still useful.

src/basic/proc-cmdline.c

keszybz · 2023-03-28T15:32:13Z

I'm not sure if #26963 is useful. I commented there.

The later parts of this PR LGTM. But needs a rebase now.

yuwata · 2023-03-29T01:04:31Z

I dropped #26963. And, the proc_cmdline_get_key_many() is re-implemented in a different way. See the first commit of this PR. PTAL!

…riend No functional change, just preparation for later commits.

When we are running in a container, we parse the command line of PID1 in proc_cmdline_parse() or friends. Previously, first we merge the command line nulstr as a single string, and then split by using extract_first_word(). That's not only redundant, but also unsafe when the command line argument contain a space. This drops the redundant steps, hence we can safely parse arguments with space.

Otherwise, if getopt() and friends are used before parse_argv(), then the GNU extensions may be ignored. This should not change any behavior at least now, as we usually use getopt_long() only once per invocation. But in the next commit, getopt_long() will be used for other arrays, hence this change will become necessary.

Otherwise, PID1 arguments e.g. "--deserialize 16" may be parsed unexpectedly by generators. Fixes the issue reported at systemd#24452 (comment).

To make ConditionKernelCommandLine= or friend not confused when we are running in a container. Addresses systemd/systemd#26887 (comment). (cherry picked from commit d2ebd50)

To make ConditionKernelCommandLine= or friend not confused when we are running in a container. Addresses systemd/systemd#26887 (comment). (cherry picked from commit d2ebd50) (cherry picked from commit 0417b28) [The patch didn't apply cleanly. When fixing stuff, I left the array size as it was. The extra few bytes don't matter and this way it's unlikely to be wrong.]

To make ConditionKernelCommandLine= or friend not confused when we are running in a container. Addresses systemd/systemd#26887 (comment). (cherry picked from commit d2ebd50) (cherry picked from commit 0417b28) [The patch didn't apply cleanly. When fixing stuff, I left the array size as it was. The extra few bytes don't matter and this way it's unlikely to be wrong.] (cherry picked from commit a85ed9a)

keszybz · 2023-04-07T08:55:26Z

LGTM.

yuwata added pid1 util-lib debug-generator labels Mar 19, 2023

github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Mar 19, 2023

This comment was marked as off-topic.

Sign in to view

mrc0mmand linked an issue Mar 19, 2023 that may be closed by this pull request

systemd-nspawn attempts to parse the deserialization FD as a runlevel, overriding the default target in some cases #24452

Closed

mrc0mmand reviewed Mar 19, 2023

View reviewed changes

src/basic/proc-cmdline.c Outdated Show resolved Hide resolved

mrc0mmand added the good-to-merge/with-minor-suggestions label Mar 19, 2023

yuwata force-pushed the proc-cmdline-filter-arguments branch from e1bb47c to 0f1eff1 Compare March 19, 2023 11:33

yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed needs-stable-backport and removed good-to-merge/with-minor-suggestions please-review PR is ready for (re-)review by a maintainer labels Mar 19, 2023

yuwata force-pushed the proc-cmdline-filter-arguments branch from 0f1eff1 to 54fd715 Compare March 19, 2023 12:08

yuwata added dont-merge 💣 and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Mar 19, 2023

yuwata force-pushed the proc-cmdline-filter-arguments branch from 54fd715 to d39f30a Compare March 19, 2023 12:55

yuwata added please-review PR is ready for (re-)review by a maintainer and removed dont-merge 💣 labels Mar 19, 2023

github-actions bot added the tests label Mar 19, 2023

YHNdnzj reviewed Mar 19, 2023

View reviewed changes

src/basic/proc-cmdline.c Outdated Show resolved Hide resolved

yuwata force-pushed the proc-cmdline-filter-arguments branch from d39f30a to e31b372 Compare March 19, 2023 13:09

mrc0mmand mentioned this pull request Mar 20, 2023

RHEL 9.2 "to-backport" list redhat-plumbers/systemd-rhel9#154

Closed

keszybz requested changes Mar 20, 2023

View reviewed changes

src/basic/proc-cmdline.c Outdated Show resolved Hide resolved

src/basic/proc-cmdline.c Outdated Show resolved Hide resolved

src/basic/proc-cmdline.c Outdated Show resolved Hide resolved

src/basic/proc-cmdline.c Outdated Show resolved Hide resolved

keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed debug-generator labels Mar 20, 2023

yuwata force-pushed the proc-cmdline-filter-arguments branch from c031a62 to 0cf6f57 Compare March 24, 2023 06:16

This was referenced Mar 24, 2023

core/main: make arguments followed by '=' #26956

Merged

proc-cmdline: several cleanups #26957

Merged

nulstr: make strv_parse_nulstr() optionally drop trailing NULs #26958

Merged

yuwata force-pushed the proc-cmdline-filter-arguments branch from 0cf6f57 to 6cee48b Compare March 24, 2023 06:30

This was referenced Mar 24, 2023

process-util: introduce get_process_cmdline_strv() #26962

Merged

proc-cmdline: reimplement proc_cmdline_get_key_many_internal() #26963

Closed

yuwata force-pushed the proc-cmdline-filter-arguments branch from 6cee48b to 65486c1 Compare March 24, 2023 12:46

keszybz added needs-rebase and removed please-review PR is ready for (re-)review by a maintainer labels Mar 28, 2023

yuwata force-pushed the proc-cmdline-filter-arguments branch from 65486c1 to 96c199d Compare March 29, 2023 01:01

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed needs-rebase labels Mar 29, 2023

yuwata added 5 commits March 29, 2023 10:33

proc-cmdline: split commandline earlier in proc_cmdline_parse() and f…

70806d4

…riend No functional change, just preparation for later commits.

condition: use proc_cmdline_strv()

dd2d3e9

proc-cmdline: filter PID1 arguments when we are running in a container

6339d3e

Otherwise, PID1 arguments e.g. "--deserialize 16" may be parsed unexpectedly by generators. Fixes the issue reported at systemd#24452 (comment).

yuwata force-pushed the proc-cmdline-filter-arguments branch from 96c199d to 6339d3e Compare March 29, 2023 01:35

yuwata added this to the v254 milestone Mar 31, 2023

keszybz merged commit ddd43f3 into systemd:main Apr 7, 2023

github-actions bot removed the please-review PR is ready for (re-)review by a maintainer label Apr 7, 2023

yuwata deleted the proc-cmdline-filter-arguments branch April 7, 2023 09:29

bluca removed the needs-stable-backport label Apr 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proc-cmdline: filter PID1 arguments on container #26887

proc-cmdline: filter PID1 arguments on container #26887

yuwata commented Mar 19, 2023

yuwata commented Mar 19, 2023

This comment was marked as off-topic.

mrc0mmand left a comment •

edited

Loading

yuwata commented Mar 19, 2023

mrc0mmand commented Mar 19, 2023

yuwata commented Mar 19, 2023

keszybz left a comment

keszybz commented Mar 28, 2023

yuwata commented Mar 29, 2023

keszybz commented Apr 7, 2023

proc-cmdline: filter PID1 arguments on container #26887

proc-cmdline: filter PID1 arguments on container #26887

Conversation

yuwata commented Mar 19, 2023

yuwata commented Mar 19, 2023

This comment was marked as off-topic.

mrc0mmand left a comment • edited Loading

Choose a reason for hiding this comment

yuwata commented Mar 19, 2023

mrc0mmand commented Mar 19, 2023

yuwata commented Mar 19, 2023

keszybz left a comment

Choose a reason for hiding this comment

keszybz commented Mar 28, 2023

yuwata commented Mar 29, 2023

keszybz commented Apr 7, 2023

mrc0mmand left a comment •

edited

Loading