On Fri, Sep 3, 2021 at 2:47 PM Kees Cook [off-list ref] wrote:

(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

[...]

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. :)

It's not used by the only current user (Android) and we can always
allow more chars later. However going the other direction and
disallowing some of them I think would be harder (need to make sure
nobody uses them). WDYT if we keep it stricter and relax if needed?

quoted

+
+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;

Yeah, I was contemplating one way or the other (less indents vs clear
flow) and you convinced me :) Will change in the next rev.
Thanks for the review!

--
Kees Cook