On Tue, Mar 15, 2022 at 01:17:06PM -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(). Hm, yes, fill_note_info() does an initial count & allocate. Then fill_thread_core_info() writes them. > > 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@xxxxxxxxx/ > > Co-developed-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx> > --- > fs/binfmt_elf.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index d61543fbd652..a48f85e3c017 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -1757,7 +1757,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t, > const struct user_regset_view *view, > long signr, size_t *total) > { > - unsigned int i; > + unsigned int note_iter, view_iter; > > /* > * NT_PRSTATUS is the one special case, because the regset data > @@ -1777,11 +1777,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t, > > /* > * 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; > @@ -1800,10 +1800,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t, > if (is_fpreg) > SET_PR_FPVALID(&t->prstatus); > info->thread_notes contains the count. Since fill_thread_core_info() passes a info member by reference, it might make sense to just pass info itself, then the size can be written and a bounds-check can be added here: if (WARN_ON_ONCE(i >= info->thread_notes)) continue; > - 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]); > + *total += notesize(&t->notes[note_iter]); > + note_iter++; > } > > return 1; > -- > 2.17.1 > If that can get adjusted, I'd be happy to carry this patch separately in for-next/execve (or I can Ack it and it can go with the others here in the series). (And in a perfect world, I'd *love* a KUnit test to exercise this logic, but I don't think we're there yet with function mocking, etc.) -- Kees Cook
