| Message ID | 20220317192013.13655-4-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Regset cleanups | expand |
On Thu, Mar 17, 2022 at 12:20:13PM -0700, Rick Edgecombe wrote: > In fill_thread_core_info() the ptrace accessible registers are collected > to be written out as notes in a core file. The note array is allocated > from a size calculated by iterating the user regset view, and counting the > regsets that have a non-zero core_note_type. However, this only allows for > there to be non-zero core_note_type at the end of the regset view. If > there are any gaps in the middle, fill_thread_core_info() will overflow the > note allocation, as it iterates over the size of the view and the > allocation would be smaller than that. > > There doesn't appear to be any arch that has gaps such that they exceed > the notes allocation, but the code is brittle and tries to support > something it doesn't. It could be fixed by increasing the allocation size, > but instead just have the note collecting code utilize the array better. > This way the allocation can stay smaller. > > Even in the case of no arch's that have gaps in their regset views, this > introduces a change in the resulting indicies of t->notes. It does not > introduce any changes to the core file itself, because any blank notes are > skipped in write_note_info(). > > In case, the allocation logic between fill_note_info() and > fill_thread_core_info() ever diverges from the usage logic, warn and skip > writing any notes that would overflow the array. > > This fix is derrived from an earlier one[0] by Yu-cheng Yu. > > [0] https://lore.kernel.org/lkml/20180717162502.32274-1-yu-cheng.yu@intel.com/ > > Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > > v2: > - Warn and break out of the note collecting loop if the allocation would > overflow. Note: I tweaked it slightly to do break instead of continue > and to do it before SET_PR_FPVALID(). (Kees) This looks great; thank you for the tweak. :) Acked-by: Kees Cook <keescook@chromium.org> Shall I take this separately into the for-next/execve tree, or would you rather is stay in this series? -Kees
On Thu, 2022-03-17 at 14:26 -0700, Kees Cook wrote: > This looks great; thank you for the tweak. :) > > Acked-by: Kees Cook <keescook@chromium.org> > > Shall I take this separately into the for-next/execve tree, or would > you > rather is stay in this series? > > -Kees Great thanks. Yea that would probably be the best I think, unless anyone speaks up.
On Thu, 17 Mar 2022 12:20:13 -0700, Rick Edgecombe wrote: > In fill_thread_core_info() the ptrace accessible registers are collected > to be written out as notes in a core file. The note array is allocated > from a size calculated by iterating the user regset view, and counting the > regsets that have a non-zero core_note_type. However, this only allows for > there to be non-zero core_note_type at the end of the regset view. If > there are any gaps in the middle, fill_thread_core_info() will overflow the > note allocation, as it iterates over the size of the view and the > allocation would be smaller than that. > > [...] Applied to for-next/execve, thanks! [3/3] elf: Don't write past end of notes for regset gap https://git.kernel.org/kees/c/dd664099002d
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index d61543fbd652..7899b42700b4 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1755,9 +1755,9 @@ static void do_thread_regset_writeback(struct task_struct *task, static int fill_thread_core_info(struct elf_thread_core_info *t, const struct user_regset_view *view, - long signr, size_t *total) + long signr, struct elf_note_info *info) { - unsigned int i; + unsigned int note_iter, view_iter; /* * NT_PRSTATUS is the one special case, because the regset data @@ -1771,17 +1771,17 @@ static int fill_thread_core_info(struct elf_thread_core_info *t, fill_note(&t->notes[0], "CORE", NT_PRSTATUS, PRSTATUS_SIZE, &t->prstatus); - *total += notesize(&t->notes[0]); + info->size += notesize(&t->notes[0]); do_thread_regset_writeback(t->task, &view->regsets[0]); /* * Each other regset might generate a note too. For each regset - * that has no core_note_type or is inactive, we leave t->notes[i] - * all zero and we'll know to skip writing it later. + * that has no core_note_type or is inactive, skip it. */ - for (i = 1; i < view->n; ++i) { - const struct user_regset *regset = &view->regsets[i]; + note_iter = 1; + for (view_iter = 1; view_iter < view->n; ++view_iter) { + const struct user_regset *regset = &view->regsets[view_iter]; int note_type = regset->core_note_type; bool is_fpreg = note_type == NT_PRFPREG; void *data; @@ -1797,13 +1797,17 @@ static int fill_thread_core_info(struct elf_thread_core_info *t, if (ret < 0) continue; + if (WARN_ON_ONCE(note_iter >= info->thread_notes)) + break; + if (is_fpreg) SET_PR_FPVALID(&t->prstatus); - fill_note(&t->notes[i], is_fpreg ? "CORE" : "LINUX", + fill_note(&t->notes[note_iter], is_fpreg ? "CORE" : "LINUX", note_type, ret, data); - *total += notesize(&t->notes[i]); + info->size += notesize(&t->notes[note_iter]); + note_iter++; } return 1; @@ -1883,7 +1887,7 @@ static int fill_note_info(struct elfhdr *elf, int phdrs, * Now fill in each thread's information. */ for (t = info->thread; t != NULL; t = t->next) - if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size)) + if (!fill_thread_core_info(t, view, siginfo->si_signo, info)) return 0; /*