Re: [PATCH v9 2/3] mm: add a field to store names for private anonymous memory
flat view
From: Kees Cook <hidden>
Date: 2021-09-03 21:47:07
Also in:
linux-fsdevel, linux-mm, lkml
(Sorry, a few more things jumped out at me when I looked again...) On Thu, Sep 02, 2021 at 04:18:12PM -0700, Suren Baghdasaryan wrote:
quoted hunk ↗ jump to hunk
[...]diff --git a/kernel/sys.c b/kernel/sys.c index 72c7639e3c98..25118902a376 100644 --- a/kernel/sys.c +++ b/kernel/sys.c@@ -2299,6 +2299,64 @@ int __weak arch_prctl_spec_ctrl_set(struct task_struct *t, unsigned long which, #define PR_IO_FLUSHER (PF_MEMALLOC_NOIO | PF_LOCAL_THROTTLE) +#ifdef CONFIG_MMU + +#define ANON_VMA_NAME_MAX_LEN 256 + +static inline bool is_valid_name_char(char ch) +{ + /* printable ascii characters, except [ \ ] */ + return (ch > 0x1f && ch < 0x5b) || (ch > 0x5d && ch < 0x7f); +}
In the back of my mind, I feel like disallowing backtick would be nice, but then if $, (, and ) are allowed, it doesn't matter, and that seems too limiting. :)
quoted hunk ↗ jump to hunk
+ +static int prctl_set_vma(unsigned long opt, unsigned long addr, + unsigned long size, unsigned long arg) +{ + struct mm_struct *mm = current->mm; + const char __user *uname; + char *name, *pch; + int error; + + switch (opt) { + case PR_SET_VMA_ANON_NAME: + uname = (const char __user *)arg; + if (!uname) { + /* Reset the name */ + name = NULL; + goto set_name; + } + + name = strndup_user(uname, ANON_VMA_NAME_MAX_LEN); + + if (IS_ERR(name)) + return PTR_ERR(name); + + for (pch = name; *pch != '\0'; pch++) { + if (!is_valid_name_char(*pch)) { + kfree(name); + return -EINVAL; + } + } +set_name: + mmap_write_lock(mm); + error = madvise_set_anon_name(mm, addr, size, name); + mmap_write_unlock(mm); + kfree(name); + break;
This is a weird construct with a needless goto. Why not:
switch (opt) {
case PR_SET_VMA_ANON_NAME:
uname = (const char __user *)arg;
if (uname) {
name = strndup_user(uname, ANON_VMA_NAME_MAX_LEN);
if (IS_ERR(name))
return PTR_ERR(name);
for (pch = name; *pch != '\0'; pch++) {
if (!is_valid_name_char(*pch)) {
kfree(name);
return -EINVAL;
}
}
} else {
/* Reset the name */
name = NULL;
}
mmap_write_lock(mm);
error = madvise_set_anon_name(mm, addr, size, name);
mmap_write_unlock(mm);
kfree(name);
break;
--
Kees Cook