On Mon, 2018-10-15 at 16:40 -0700, Kees Cook wrote:

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

quoted

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

[...]

quoted

+/*
+ * The .note.gnu.property layout:
+ *
+ *     struct elf_note {
+ *             u32 n_namesz; --> sizeof(n_name[]); always (4)
+ *             u32 n_ndescsz;--> sizeof(property[])
+ *             u32 n_type;   --> always NT_GNU_PROPERTY_TYPE_0
+ *     };
+ *     char n_name[4]; --> always 'GNU\0'
+ *
+ *     struct {
+ *             struct property_x86 {
+ *                     u32 pr_type;
+ *                     u32 pr_datasz;
+ *             };
+ *             u8 pr_data[pr_datasz];
+ *     }[];
+ */

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

There are other property types, but we only look for NT_GNU_PROPERTY_TYPE_0.

quoted

+
+#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...

Yes, Thanks!

quoted

+}
+
+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.

quoted

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

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

Note->n_namesz is always four-byte.  I should have used u32.

quoted

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

Here we only detect the warp around.  After this returns we then check other
types of overflow in scan().

quoted

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

Yes, they need to be in ascending order.

quoted

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

quoted

+           (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.

I need to work on that :-)  What would you suggest?

quoted

+{
+       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?

There are other property types, but the kernel does not look at most of them.
If the kernel needs to look at others, we need to rewrite this.

[...]

quoted

+
+/*
+ * 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?)

BUF_SIZE can be smaller, for example 64.  If it is too small, we need to do
kernel_read() too often.

quoted

+
+       *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...

quoted

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

We need to wait until SET_PERSONALITY2() is done.

[...]

quoted

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

Yes, thanks.

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.

If the item is a bit of GNU_PROPERTY_X86_FEATURE_1_AND, then this code would
work.  Has it been finalized?

Yu-cheng