lib/alloc_tag.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
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().
Fixes: 9f44df50fee4 ("alloc_tag: keep codetag iterator active between read()")
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
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--;
+
+ return ct ? priv : NULL;
}
static void *allocinfo_next(struct seq_file *m, void *arg, loff_t *pos)
--
2.25.1On Mon, May 25, 2026 at 12:22 AM Hao Ge <hao.ge@linux.dev> 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@linux.dev>
> ---
> 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
>
On 2026/6/4 02:59, Suren Baghdasaryan wrote:
> On Mon, May 25, 2026 at 12:22 AM Hao Ge <hao.ge@linux.dev> 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@linux.dev>
>> ---
>> 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?
Hi Suren
Thanks for the suggestion, I completely agree.
My patch has exactly the problem you described - reinitializing from
scratch on every
start() also has an O(n) performance problem: each start(pos=N) needs to
skip N
elements from the beginning, regardless of whether any module was unloaded.
One small difference from your suggestion: I save the iterator state in
allocinfo_next() rather than allocinfo_show().
Since seq_read_iter() may roll back show()'s output on overflow
https://elixir.bootlin.com/linux/v7.1-rc6/source/fs/seq_file.c#L276
saving in show() could capture an element whose output was discarded,
causing it to be skipped on the next start().
In the Fill loop, next() is not called again after a show() overflow
since seq_read_iter() breaks out immediately.
Will send a v2 shortly.
Thanks
Best Regards
Hao
>> +
>> + return ct ? priv : NULL;
>> }
>>
>> static void *allocinfo_next(struct seq_file *m, void *arg, loff_t *pos)
>> --
>> 2.25.1
>>
© 2016 - 2026 Red Hat, Inc.