Messages in this thread | | | From | Kees Cook <> | Date | Tue, 5 Dec 2017 12:32:22 -0800 | Subject | Re: [PATCH] ocfs2: use get_task_comm |
| |
On Tue, Dec 5, 2017 at 12:27 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Dec 5, 2017 at 8:19 PM, Kees Cook <keescook@chromium.org> wrote: >> On Tue, Dec 5, 2017 at 7:20 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>> While reviewing all callers of get_task_comm(), I stumbled >>> over this one that claimed it was not exported, when in fact >>> it is. Accessing task->comm directly is not safe, so better >>> convert this one to using get_task_comm as well. >> >> Using get_task_comm() in cases like this is actually overkill (i.e. >> using up stack space), since there's (currently) no benefit. Nothing >> protects getting a "correct" view of task->comm (i.e. it could get >> updated in the middle of a copy), but it _is_ always NULL terminated, >> so it's safe to use with %s like this. While it does make me slightly >> uncomfortable to _depend_ on this NULL termination, but there are lots >> of open-coded %s users of task->comm. When we're trying to save a >> _copy_ of task->comm, then we want get_task_comm(), just to make sure >> we're doing it right. >> >> So, while I don't oppose this patch, it might be seen as a wasteful >> use of stack space. > > It's only a few bytes of stack space in a leaf function, I'd not be > worried about that. > > More generally speaking though, how exactly do we guarantee that > there is NUL-termination on tsk->comm during a concurrent update? > Could we ever get into a situation where overwrite the NUL byte > while setting tsk->comm to a longer string, and read the new start > of the string together with an unterminated end, or do we strictly > guarantee that the last byte is still NUL? I assume the latter is > true, just haven't found exactly where that guarantee is made.
strncpy will zero pad with the trailing NULL, so it's supposed to always be safe... still gives me the creeps, though.
-Kees
-- Kees Cook Pixel Security
| |