| Message ID | 20220315201706.7576-4-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Regset cleanups | expand |
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@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> > --- > 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.)
On Tue, 2022-03-15 at 13:37 -0700, Kees Cook wrote: > > /* > > * 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; Hi Kees, Thanks for the quick response. Are you saying in addition to utilizing the allocation better, also catch if the allocation is still too small? Or do this check instead of the change in how to utilize the array, and then maintain the restriction on having gaps in the regsets? If it's the former, it seems a bit excessive since the allocation and usage are only one function call away from each other and the logic is now such that it can't overflow. I can add it if you want. If it's to just warn on the gaps, it could also be done directly like: /* Don't expect gaps in regset views */ WARN_ON(!regset->regset_get); And it might be a little clearer of a hint about this expectation of the arch's. Let me know what you prefer and I can make the change.
On Tue, Mar 15, 2022 at 09:48:29PM +0000, Edgecombe, Rick P wrote: > On Tue, 2022-03-15 at 13:37 -0700, Kees Cook wrote: > > > /* > > > * 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; > > Hi Kees, > > Thanks for the quick response. > > Are you saying in addition to utilizing the allocation better, also > catch if the allocation is still too small? Or do this check instead of > the change in how to utilize the array, and then maintain the > restriction on having gaps in the regsets? What I want is to have writers of dynamically-sized arrays able to do the bounds check in the same place the write happens, so passing "info" makes sense to me.
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); - 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;