linux-kernel - Re: [PATCH] mm/kmemleak: Fix sleeping function called from invalid context in kmemleak_seq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z0BZFW1jdIwpFrGz@arm.com>
Date: Fri, 22 Nov 2024 10:12:37 +0000
From: Catalin Marinas <catalin.marinas@....com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	Alessandro Carminati <acarmina@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Clark Williams <clrkwllms@...nel.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
	Thomas Weissschuh <thomas.weissschuh@...utronix.de>,
	Alessandro Carminati <alessandro.carminati@...il.com>,
	Juri Lelli <juri.lelli@...hat.com>,
	Gabriele Paoloni <gpaoloni@...hat.com>,
	Eric Chanudet <echanude@...hat.com>
Subject: Re: [PATCH] mm/kmemleak: Fix sleeping function called from invalid
 context in kmemleak_seq_show

On Fri, Nov 22, 2024 at 09:14:37AM +0100, Sebastian Andrzej Siewior wrote:
> On 2024-11-21 19:19:05 [+0000], Catalin Marinas wrote:
> …
> > > Maybe SELinux locks should be converted to raw? I don't know how long that
> > > lock is held. There are some loops though :-/
> > > 
> > > avc_insert():
> > > 
> > > 	spin_lock_irqsave(lock, flag);
> > > 	hlist_for_each_entry(pos, head, list) {
> > > 		if (pos->ae.ssid == ssid &&
> > > 			pos->ae.tsid == tsid &&
> > > 			pos->ae.tclass == tclass) {
> > > 			avc_node_replace(node, pos);
> > > 			goto found;
> > > 		}
> > > 	}
> > > 	hlist_add_head_rcu(&node->list, head);
> > > found:
> > > 	spin_unlock_irqrestore(lock, flag);
> > > 
> > > Perhaps that could be converted to simple RCU?
> > > 
> > > As I'm sure there's other places that call vsprintf() under a raw_spin_lock
> > > or non-preemptable context, perhaps this should be the fix we do.
> > 
> > My preference would also be to convert SELinux rather than avoiding the
> > issue in kmemleak (and other similar places).
> 
> No. kmemleak has been made use a raw_spinlock_t because most of what it
> does is something that is not used in production on a PREEMPT_RT system
> and falls in the same category as lockdep for instance. And that code
> calls into LSM/ selinux.
> Before making the lock in selinux a raw_spinlock_t you have to think
> about the consequences in general and audit the code. From a quick
> look, there is also avc_insert() invoked in that callchain which
> allocates memory and this is a no no.
> Also, if you make the solution here in selinux to use a raw_spinlock_t
> you would have to do it also in every LSM as they might be used instead
> of selinux.

Good point, thanks. Kmemleak is indeed a debug tool not supposed to be
used in production. Modifying SELinux has wider implications for
PREEMPT_RT.

> Therefore, I still prefer adding PREEMPT_RT to the restricted_pointer()
> category for atomic invocations.

This should work. If one wants the actual (hashed) pointers with
kmemleak, I guess they can disable kptr_restrict.

-- 
Catalin