On Mi, 16.05.18 11:34, Linus Torvalds (torvalds@xxxxxxxxxxxxxxxxxxxx) wrote: > 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. The main reason we filter out non-isprint()able characters for get_process_cmdline() but not for get_process_comm() is that there are various tools which rewrite their cmdline to show some metadata about what they are doing. Traditionally there was no way to change the size of the cmdline buffer, so they resorted to filling it up with NUL bytes (and some even worse, relying that some tools would stop processing cmdline after a double NUL even if there was more data following — other tools otoh do not). Hence we filter non-printable bytes, and in particular reduce sequences of NUL bytes to single spaces. I figure we could filter the comm stuff too for non-displayable chars, but then again, maybe it's Ok the way it is, it's a low-level interface after all, and maybe such filtering should take place at display times, dunno. IIRC Last time I looked ps/top and friends don't filter either though? > This is buggy and insecure for two reasons: > > - you can trick systemd into thinking a process is a kernel process by > just unmapping argv[]. We don't do things this way anymore, as you already found out. We copied the original "is this a kernel thread" logic from "ps". I have recently updated this to make this more strict, and properly check for kernel threads. The kernel APIs for all this aren't really good though, and I always was reluctant to check for PF_KTHREAD, as that flag is neither documented for userspace, nor available in any userspace-accessible headers. However, I wanted to tighten this a bit, and hence we now define the flag in our own code, as it appeared to me otherwise there was no chance to ever make this fully robust. I am not too concerned about all of this I must say though. At the point where we care if something is a kernel thread or not, unpriv userspace should long be gone, hence any attacks in this regard so far appeared mostly theoretical to me... > 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. I think filtering non-printable chars from comm (and cmdline if possible) already in the kernel would be wise. Tons of people use `cat /proc/$PID/comm` in their shell scripts, and if they do non-filtered chars have a good chance to be useful for exploits. Lennart -- Lennart Poettering, Red Hat -- 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
