On Fri, Sep 21, 2018 at 8:03 AM, Yu-cheng Yu [off-list ref] wrote:

Look in .note.gnu.property of an ELF file and check if Shadow Stack needs
to be enabled for the task.

Ah, I've been wanting this for other things too (see below).

Does NT_GNU_PROPERTY_TYPE_0 only ever contain property_x86 bytes? (I
assume not, since there is a pr_type?)

+
+#define BUF_SIZE (PAGE_SIZE / 4)
+
+struct property_x86 {
+       u32 pr_type;
+       u32 pr_datasz;
+};
+
+typedef bool (test_fn)(void *buf, u32 *arg);
+typedef void *(next_fn)(void *buf, u32 *arg);
+
+static inline bool test_note_type_0(void *buf, u32 *arg)
+{
+       struct elf_note *n = buf;
+
+       return ((n->n_namesz == 4) && (memcmp(n + 1, "GNU", 4) == 0) &&
+               (n->n_type == NT_GNU_PROPERTY_TYPE_0));

Cheaper to test n_type first...

+}
+
+static inline void *next_note(void *buf, u32 *arg)
+{
+       struct elf_note *n = buf;
+       u32 align = *arg;
+       int size;
+
+       size = round_up(sizeof(*n) + n->n_namesz, align);

I think this could overflow: n_namesz can be u64 for elf64_note.

+       size = round_up(size + n->n_descsz, align);

Same here. You may want to use check_add_overflow(), etc, an u64 types.

+
+       if (buf + size < buf)
+               return NULL;

I don't understand this. You want to check size not exceeding the
allocation, which isn't passed into this function. Checking for a full
unsigned address wrap around is not sufficient to detect overflow.

+       else
+               return (buf + size);
+}
+
+static inline bool test_property_x86(void *buf, u32 *arg)
+{
+       struct property_x86 *pr = buf;
+       u32 max_type = *arg;
+
+       if (pr->pr_type > max_type)
+               *arg = pr->pr_type;

Why is *arg being updated? I don't see last_pr used outside of here --
are properties required to be pr_type-ordered?

+
+       return (pr->pr_type == GNU_PROPERTY_X86_FEATURE_1_AND);
+}
+
+static inline void *next_property(void *buf, u32 *arg)
+{
+       struct property_x86 *pr = buf;
+       u32 max_type = *arg;
+
+       if ((buf + sizeof(*pr) +  pr->pr_datasz < buf) ||

Again, this "< buf" test doesn't look at all correct to me.

+           (pr->pr_type > GNU_PROPERTY_X86_FEATURE_1_AND) ||
+           (pr->pr_type > max_type))
+               return NULL;
+       else
+               return (buf + sizeof(*pr) + pr->pr_datasz);
+}
+
+/*
+ * Scan 'buf' for a pattern; return true if found.
+ * *pos is the distance from the beginning of buf to where
+ * the searched item or the next item is located.
+ */
+static int scan(u8 *buf, u32 buf_size, int item_size,
+                test_fn test, next_fn next, u32 *arg, u32 *pos)

I'm not a fan of the short "scan", "test" and "next" names, and I
really don't like an arg named "arg". Something slightly more
descriptive for all of these would be nice, please.

+{
+       int found = 0;
+       u8 *p, *max;
+
+       max = buf + buf_size;
+       if (max < buf)
+               return 0;
+
+       p = buf;
+
+       while ((p + item_size < max) && (p + item_size > buf)) {

These comparisons are safe due to the BUF_SIZE limit of buf_size and
the only used size of item_size, but if this becomes more generic, it
should be more defensive on the size calculations (e.g. make sure than
"item_size < max" and then here "p < max - item_size", etc).

I'd kind of rather this code walked the base type and check each for
the matching feature. What is the general specification for what
NT_GNU_PROPERTY_TYPE_0 contains?

+               if (test(p, arg)) {
+                       found = 1;
+                       break;
+               }
+
+               p = next(p, arg);
+       }
+
+       *pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);
+       return found;
+}
+
+/*
+ * Search a NT_GNU_PROPERTY_TYPE_0 for GNU_PROPERTY_X86_FEATURE_1_AND.
+ */
+static int find_feature_x86(struct file *file, unsigned long desc_size,
+                           loff_t file_offset, u8 *buf, u32 *feature)
+{
+       u32 buf_pos;
+       unsigned long read_size;
+       unsigned long done;
+       int found = 0;
+       int ret = 0;
+       u32 last_pr = 0;
+
+       *feature = 0;
+       buf_pos = 0;
+
+       for (done = 0; done < desc_size; done += buf_pos) {
+               read_size = desc_size - done;
+               if (read_size > BUF_SIZE)
+                       read_size = BUF_SIZE;
+
+               ret = kernel_read(file, buf, read_size, &file_offset);
+
+               if (ret != read_size)
+                       return (ret < 0) ? ret : -EIO;
+
+               ret = 0;
+               found = scan(buf, read_size, sizeof(struct property_x86),
+                            test_property_x86, next_property,
+                            &last_pr, &buf_pos);
+
+               if ((!buf_pos) || found)
+                       break;
+
+               file_offset += buf_pos - read_size;
+       }
+
+       if (found) {
+               struct property_x86 *pr =
+                       (struct property_x86 *)(buf + buf_pos);
+
+               if (pr->pr_datasz == 4) {
+                       u32 *max =  (u32 *)(buf + read_size);
+                       u32 *data = (u32 *)((u8 *)pr + sizeof(*pr));
+
+                       if (data + 1 <= max) {
+                               *feature = *data;
+                       } else {
+                               file_offset += buf_pos - read_size;
+                               file_offset += sizeof(*pr);
+                               ret = kernel_read(file, feature, 4,
+                                                 &file_offset);
+                       }
+               }
+       }
+
+       return ret;
+}
+
+/*
+ * Search a PT_NOTE segment for the first NT_GNU_PROPERTY_TYPE_0.
+ */
+static int find_note_type_0(struct file *file, unsigned long note_size,
+                           loff_t file_offset, u32 align, u32 *feature)
+{
+       u8 *buf;
+       u32 buf_pos;
+       unsigned long read_size;
+       unsigned long done;
+       int found = 0;
+       int ret = 0;
+
+       buf = kmalloc(BUF_SIZE, GFP_KERNEL);
+       if (!buf)
+               return -ENOMEM;

Why kmalloc over stack variable? (Or, does BUF_SIZE here really need
to be 1024?)

+
+       *feature = 0;
+       buf_pos = 0;
+
+       for (done = 0; done < note_size; done += buf_pos) {
+               read_size = note_size - done;
+               if (read_size > BUF_SIZE)
+                       read_size = BUF_SIZE;
+
+               ret = kernel_read(file, buf, read_size, &file_offset);
+
+               if (ret != read_size) {
+                       ret = (ret < 0) ? ret : -EIO;
+                       kfree(buf);
+                       return ret;
+               }
+
+               /*
+                * item_size = sizeof(struct elf_note) + elf_note.n_namesz.
+                * n_namesz is 4 for the note type we look for.
+                */
+               ret = 0;
+               found += scan(buf, read_size, sizeof(struct elf_note) + 4,
+                             test_note_type_0, next_note,
+                             &align, &buf_pos);
+
+               file_offset += buf_pos - read_size;
+
+               if (found == 1) {
+                       struct elf_note *n =
+                               (struct elf_note *)(buf + buf_pos);
+                       u32 start = round_up(sizeof(*n) + n->n_namesz, align);
+                       u32 total = round_up(start + n->n_descsz, align);

Same overflow notes from earlier...

+
+                       ret = find_feature_x86(file, n->n_descsz,
+                                              file_offset + start,
+                                              buf, feature);
+                       file_offset += total;
+                       buf_pos += total;
+               } else if (!buf_pos) {
+                       *feature = 0;
+                       break;
+               }
+       }
+
+       kfree(buf);
+       return ret;
+}
+
+#ifdef CONFIG_COMPAT
+static int check_notes_32(struct file *file, struct elf32_phdr *phdr,
+                         int phnum, u32 *feature)
+{
+       int i;
+       int err = 0;
+
+       for (i = 0; i < phnum; i++, phdr++) {
+               if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 4))
+                       continue;
+
+               err = find_note_type_0(file, phdr->p_filesz, phdr->p_offset,
+                                      phdr->p_align, feature);
+               if (err)
+                       return err;
+       }
+
+       return 0;
+}
+#endif
+
+#ifdef CONFIG_X86_64
+static int check_notes_64(struct file *file, struct elf64_phdr *phdr,
+                         int phnum, u32 *feature)
+{
+       int i;
+       int err = 0;
+
+       for (i = 0; i < phnum; i++, phdr++) {
+               if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 8))
+                       continue;

Instead of a separate parser here, wouldn't it be a bit nicer to
attach this to the existing binfmt_elf program header parsing loop:

        elf_ppnt = elf_phdata;
        for (i = 0; i < loc->elf_ex.e_phnum; i++, elf_ppnt++)
                switch (elf_ppnt->p_type) {
                case PT_GNU_STACK:
...
                case PT_LOPROC ... PT_HIPROC:
...

+
+               err = find_note_type_0(file, phdr->p_filesz, phdr->p_offset,
+                                      phdr->p_align, feature);
+               if (err)
+                       return err;
+       }
+
+       return 0;
+}
+#endif
+
+int arch_setup_features(void *ehdr_p, void *phdr_p,
+                       struct file *file, bool interp)
+{
+       int err = 0;
+       u32 feature = 0;
+
+       struct elf64_hdr *ehdr64 = ehdr_p;
+
+       if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
+               return 0;
+
+       if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
+               struct elf64_phdr *phdr64 = phdr_p;
+
+               err = check_notes_64(file, phdr64, ehdr64->e_phnum,
+                                    &feature);
+               if (err < 0)
+                       goto out;
+       } else {
+#ifdef CONFIG_COMPAT
+               struct elf32_hdr *ehdr32 = ehdr_p;
+
+               if (ehdr32->e_ident[EI_CLASS] == ELFCLASS32) {
+                       struct elf32_phdr *phdr32 = phdr_p;
+
+                       err = check_notes_32(file, phdr32, ehdr32->e_phnum,
+                                            &feature);
+                       if (err < 0)
+                               goto out;
+               }
+#endif

Should there be an #else error here?

+       }
+
+       memset(&current->thread.cet, 0, sizeof(struct cet_status));
+
+       if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {

The CPU feature was already tested at arch_setup_features() entry.

I'd like to be using this code for a few other cases too (not just
x86-specific). For example, for marking KASan binaries as needing a
"legacy" memory layouts[1]. Others might be setting things like
no_new_privs at exec time, etc.

-Kees

[1] https://lkml.kernel.org/r/CAGXu5jL1HRG7Dn9vraw8Hu7LF+69k3EDpztt1Ju7ijEzmvRdhA@mail.gmail.com

-- 
Kees Cook
Pixel Security