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

[Suren Baghdasaryan]

On Mon, May 25, 2026 at 12:22 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.
>
> Fix by always reinitializing the iterator in allocinfo_start().

Thanks for the fix!
Yeah, unfortunately with the way seq_read_iter() is implemented, the
op->next() fetches the next element and then seq_has_overflowed()
might cause op->stop() without printing that element. So, in the
op->start() that follows we can't just call codetag_next_ct(), which
would handle the module unloading case (see
https://elixir.bootlin.com/linux/v7.0.1/source/lib/codetag.c#L93).

>
> Fixes: 9f44df50fee4 ("alloc_tag: keep codetag iterator active between read()")
> Signed-off-by: Hao Ge <hao.ge@xxxxxxxxx>
> ---
>  lib/alloc_tag.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> index ed1bdcf1f8ab..2b2d1580c714 100644
> --- a/lib/alloc_tag.c
> +++ b/lib/alloc_tag.c
> @@ -51,16 +51,19 @@ struct allocinfo_private {
>  static void *allocinfo_start(struct seq_file *m, loff_t *pos)
>  {
>         struct allocinfo_private *priv;
> +       struct codetag *ct;
>         loff_t node = *pos;
>
>         priv = (struct allocinfo_private *)m->private;
>         codetag_lock_module_list(alloc_tag_cttype, true);
> -       if (node == 0) {
> +       if (node == 0)
>                 priv->print_header = true;
> -               priv->iter = codetag_get_ct_iter(alloc_tag_cttype);
> -               codetag_next_ct(&priv->iter);
> -       }
> -       return priv->iter.ct ? priv : NULL;
> +
> +       priv->iter = codetag_get_ct_iter(alloc_tag_cttype);
> +       while ((ct = codetag_next_ct(&priv->iter)) != NULL && node)
> +               node--;

I don't quite like this fix because if a module that we already passed
gets unloaded too, then skipping "node" count of elements might skip
some extra valid lines that we did not report yet. That's why
codetag_next_ct() directly goes to the next module using
idr_get_next_ul(). I would rather store the last codetag reported
using allocinfo_show() (last_reported) and use
codetag_next_ct(last_reported) in the allocinfo_start() to go to the
next one (or possibly skip to the next module). Does that make sense?

> +
> +       return ct ? priv : NULL;
>  }
>
>  static void *allocinfo_next(struct seq_file *m, void *arg, loff_t *pos)
> --
> 2.25.1
>