lkml.org 
[lkml]   [2018]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH v4 21/27] x86/cet/shstk: ELF header parsing of Shadow Stack
    From
    Date
    On Mon, 2018-10-15 at 16:40 -0700, Kees Cook wrote:
    > On Fri, Sep 21, 2018 at 8:03 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
    > > Look in .note.gnu.property of an ELF file and check if Shadow Stack needs
    > > to be enabled for the task.

    [...]

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

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

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

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

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

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

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

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

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

    [...]

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

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

    We need to wait until SET_PERSONALITY2() is done.

    [...]

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

    \
     
     \ /
      Last update: 2018-10-16 19:29    [W:4.940 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site