Re: [PATCH v2] alloc_tag: fix use-after-free in /proc/allocinfo after module unload

[Suren Baghdasaryan]

On Fri, Jun 5, 2026 at 12:44 AM Hao Ge <hao.ge@xxxxxxxxx> wrote:
>
> Hi Suren
>
>
> On 2026/6/5 03:24, Suren Baghdasaryan wrote:
> > On Thu, Jun 4, 2026 at 12:00 AM Hao Ge <hao.ge@xxxxxxxxx> wrote:
> >> allocinfo_start() only reinitializes the codetag iterator at position 0.
> >> For subsequent reads (position > 0), it reuses cached iterator state from
> >> the previous batch.  allocinfo_stop() drops mod_lock between read batches,
> >> which allows module unload to complete and free the module memory that the
> >> cached iterator still references:
> >>
> >>    CPU0 (read)                        CPU1 (rmmod)
> >>    ----                               ----
> >>    allocinfo_start(pos=0)
> >>      down_read(mod_lock)
> >>      allocinfo_show()
> >>      ...
> >>    allocinfo_stop()
> >>      up_read(mod_lock)
> >>                                       codetag_unload_module()
> >>                                         kfree(cmod)
> >>                                         release_module_tags()
> >>                                       ...
> >>                                       free_mod_mem()
> >>    allocinfo_start(pos=N)
> >>      down_read(mod_lock)
> >>      // reuses cached iter, skips re-init
> >>    allocinfo_show()
> >>      ct->filename   <-- UAF
> >>
> >> After free_mod_mem() frees the module's .rodata, allocinfo_show()
> >> dereferences ct->filename, ct->function which point there.
> >>
> >> Save the iterator state in allocinfo_next() and resume from it in
> >> allocinfo_start() with codetag_next_ct(), which detects module removal
> >> via idr_find() returning NULL and skips to the next module.
> >>
> >> Fixes: 9f44df50fee4 ("alloc_tag: keep codetag iterator active between read()")
> >> Suggested-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> >> Signed-off-by: Hao Ge <hao.ge@xxxxxxxxx>
> > Acked-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> >
> > Thanks!
> >
> >> ---
> >> v2:
> >> - save the iterator state in allocinfo_next() and resume from it in
> >>    allocinfo_start() with codetag_next_ct(), which detects module removal
> >>    via idr_find() returning NULL and skips to the next module (Suggested
> >>    by Suren).
> >>    v1 link: https://lore.kernel.org/all/20260525072117.112779-1-hao.ge@xxxxxxxxx/
> >> ---
> >>   lib/alloc_tag.c | 9 +++++++--
> >>   1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> >> index f2f574bcf383..551cc14bb1fd 100644
> >> --- a/lib/alloc_tag.c
> >> +++ b/lib/alloc_tag.c
> >> @@ -45,6 +45,7 @@ int alloc_tag_ref_offs;
> >>
> >>   struct allocinfo_private {
> >>          struct codetag_iterator iter;
> >> +       struct codetag_iterator reported_iter;
> > nit: I would call it maybe prev_iter since it's not yet reported (you
> > store it in the allocinfo_next(), not in allocinfo_show()) but that's
> > a minor detail.
> I see...
> >>          bool print_header;
> >>   };
> >>
> >> @@ -58,16 +59,20 @@ static void *allocinfo_start(struct seq_file *m, loff_t *pos)
> >>          if (node == 0) {
> >>                  priv->print_header = true;
> >>                  priv->iter = codetag_get_ct_iter(alloc_tag_cttype);
> >> -               codetag_next_ct(&priv->iter);
> >> +       } else {
> >> +               priv->iter = priv->reported_iter;
> >>          }
> >> +       codetag_next_ct(&priv->iter);
> >>          return priv->iter.ct ? priv : NULL;
> >>   }
> >>
> >>   static void *allocinfo_next(struct seq_file *m, void *arg, loff_t *pos)
> >>   {
> >>          struct allocinfo_private *priv = (struct allocinfo_private *)arg;
> >> -       struct codetag *ct = codetag_next_ct(&priv->iter);
> >> +       struct codetag *ct;
> >>
> >> +       priv->reported_iter = priv->iter;
> >> +       ct = codetag_next_ct(&priv->iter);
>
> AFAIK, in the seq_file call flow (start -> show -> next -> show -> next
> -> ...):
>
> start(0): iter = get_ct_iter(); next_ct() -> iter.ct = A
>
> show():   dump entry with iter.ct = A
>
> next():   reported_iter = iter(A); next_ct() -> iter.ct = B
>
> show():   dump entry with iter.ct = B
>
> next():   reported_iter = iter(B); next_ct() -> iter.ct = C
>
> show():   dump entry with iter.ct = C
>
> The same holds for the overflow case in the Fill loop:
>
> next():   reported_iter = iter(C); next_ct() -> iter.ct = D
>
> show():   overflow, output rolled back, break
>
> stop()
>
> on the next read:
>
> start():  iter = reported_iter(C); next_ct() -> iter.ct = D
>
> show():   dump entry with iter.ct = D
>
> So reported_iter always points to the last element that has been
>
> shown to the user.

Yes, you are correct. I thought there was a case when we move to the
next but then do not show because of the overflow but upon revisiting
that code I see that is not the case. All good.
Thanks!

>
> I'm fine with prev_iter as well,  What do you think?
>
>
> Thanks
>
> Best Regards
>
> Hao
>
>
> >>          (*pos)++;
> >>          if (!ct)
> >>                  return NULL;
> >> --
> >> 2.25.1
> >>