Re: [PATCH v9 2/3] mm: add a field to store names for private anonymous memory
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. :)
+
+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