From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-179.mta0.migadu.com (out-179.mta0.migadu.com [91.218.175.179]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DB3D63D891F for ; Thu, 4 Jun 2026 06:45:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.179 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780555567; cv=none; b=JDug/Be5ImoRrEN2gULWd7Im+CuFXyPx1FQ4j38LXVTFSN8iwcIb++PNczd2VqcsJsLlSekJmKkAge17s+mcu0ZUHIIcCJDped4MChRtIgNU5BTbDora1tb/CXAU0+WS9WAtLPNjt8z+VFGW1Nm2hALxrZdzAG5y16j9N9EwMRc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780555567; c=relaxed/simple; bh=oSKh6hLfKAfD4J3soLP3i4qyeIuUn0Fjzueddc/Ix0M=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=NBJKJgnFMKk8QibR6/XfEy3RDpZpea6q/udNqt4TRRXcNIysXz6DURpwLgwzSSbvqtvYdiW33111D7KAt12Eln6mZX/nju7C9gemrjq1Zm10WaAzS5Kb8WRhAsp3KTra0dZ53V6b11p3aLsd5z55XPy5KwtYpI90OKWPw0zjOdY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=FNRZSfnc; arc=none smtp.client-ip=91.218.175.179 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="FNRZSfnc" Message-ID: <02ae706c-0fa2-488a-8530-b6da8daf39ca@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1780555555; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KhIU+QoUu5iwz8e1xyKoqpvvqNCKUWZqivCuhNU2rC0=; b=FNRZSfncCTtCtDhmojfDOjkb8DdFyeYZDPH0LX4m3b/8ClYV4sV5MHLh4fiy4pPptsGnpV 505Acmfftk3PWAv2uXjs6PV1qSa3gPcI2Cyan+ZlNdkauR3pagZkKtP5wxZwOC/PxTdw1R AwRfff08EjJaLC2wHdVeq2catRUkZTQ= Date: Thu, 4 Jun 2026 14:45:17 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH] alloc_tag: fix use-after-free in /proc/allocinfo after module unload To: Suren Baghdasaryan Cc: Kent Overstreet , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20260525072117.112779-1-hao.ge@linux.dev> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Hao Ge In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT On 2026/6/4 02:59, Suren Baghdasaryan wrote: > On Mon, May 25, 2026 at 12:22 AM Hao Ge 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 >> --- >> 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 >>