On Wed, May 16, 2018 at 10:35 AM Linus Torvalds <
torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> Yeah, so what the tools seem to do is
> - use /proc/<pid>/cmdline for normal stuff
> - if that is empty, use the /proc/stat information.
Ok, looking at the source code, systemd does:
- if /proc/<pid>/cmdline is empty, assume kernel thread
- use /proc/<pid>/comm to get the thread name and surround it by "[]".
and it's worth noting that systemd also does not do any character escaping
for the /proc/<pid>/comm case. That's safe *if* it's a kernel thread, but
since that's not a given to begin with, the code is wrong.
This is buggy and insecure for two reasons:
- you can trick systemd into thinking a process is a kernel process by
just unmapping argv[].
Oddly, "is_kernel_thread()" gets this right. It seems to just read the
"flags" field in /proc/<pid>/stat, so I'm not sure why you get different
behavior at shutdown with your kworker patch.
I'm looking at current -git of systemd, so maybe this is a bug that
systemd _used_ to have. Or maybe there is some other thing that looks at
cmdline than the src/basic/process-util.c code I'm looking at.
- you can trick systemd into skipping the "isprint()" logic in
get_process_cmdline by making the real executable have some odd name (in
addition to the unmapping of argv[]).
I would suspect that we can try to help this buggy systemd situation two
ways:
- for non-kernel threads, we could always fill in /proc/<pid>/cmdline with
the contents of 'comm[]' if we cannot return any data from the actual user
space mapping.
- make sure that __set_task_comm() does that 'isprint()' so that we never
expose unprintable characters if somebody does something fishy.
Hmm?
I didn't look at what 'ps' does, but I would not be surprised if there is
some code sharing between the two.
Oh, looking at systemd git history, it turns out that the buggy behavior
wrt cmdline was fixed in February of this year.
So systemd got fixed recently wrt the "is_kernel_thread()" thing, but the
"skip nonprintable characters" code when falling back to /proc/<pid>/comm
is still buggy.
Maybe the unprintable characters aren't a big deal.
Adding Lennart to the cc, since he fixed the "is_kernel_thread()" thing,
and presumably knows if the unprintable characters really matter or not.
Maybe it is just a UI issue, not a "potentially confuse systemd" issue.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
