Message-ID: <20250303115007-beb39d5b-71f5-458a-82fe-9e82c9ab720e@linutronix.de>
Date: Mon, 3 Mar 2025 12:08:11 +0100
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>, 
	Mahesh J Salgaonkar <mahesh@...ux.ibm.com>, Oliver O'Halloran <oohall@...il.com>, 
	Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>, 
	Nicholas Piggin <npiggin@...il.com>, Naveen N Rao <naveen@...nel.org>, linuxppc-dev@...ts.ozlabs.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc: Don't use %pK through printk

On Fri, Feb 28, 2025 at 08:15:02PM +0000, Maciej W. Rozycki wrote:
> On Wed, 26 Feb 2025, Thomas Weißschuh wrote:
> 
> > > > By default, when kptr_restrict is set to 0, %pK behaves the same as %p.
> > > > The same happened for a bunch of other architectures and nobody seems
> > > > to have noticed in the past.
> > > > The symbol-relative pointers or pointer formats designed for backtraces,
> > > > as notes by Christophe, seem to be enough.
> > > 
> > >  I do hope so.
> > 
> > As mentioned before, personally I am fine with using %px here.
> 
>  Glad to hear!
> 
> > The values are in the register dumps anyways and security sensitive deployments
> > will panic on WARN(), making the information disclosure useless.
> 
>  And even more so, I wasn't aware of this feature.  But this code doesn't 
> make use of the WARN() facility, it just prints at the heightened KERN_ERR 
> priority.

Indeed, I got confused with some other patches where WARN() is used mostly.
This makes it a bit murkier.

> > > > But personally I'm also fine with using %px, as my goal is to remove the
> > > > error-prone and confusing %pK.
> > > 
> > >  It's clear that `%pK' was meant to restrict access to /proc files and the 
> > > like that may be accessible by unprivileged users:
> > 
> > Then let's stop abusing it. For something that is clear, it is
> > misunderstood very often.
> 
>  Absolutely, I haven't questioned the removal of `%pK', but the switch to 
> `%p' rather than `%px' specifically for this single hunk of your patch.

Sure. It would be great if one of the maintainers could confirm this preference.

> > > "
> > > kptr_restrict
> > > =============
> > > 
> > > This toggle indicates whether restrictions are placed on
> > > exposing kernel addresses via ``/proc`` and other interfaces.
> > > "
> > > 
> > > and not the kernel log, the information in which may come from rare events 
> > > that are difficult to trigger and hard to recover via other means.  Sigh. 
> > > Once you've got access to the kernel log, you may as well wipe the system 
> > > or do any other harm you might like.
> > 
> > As I understand it, both the security and printk maintainers don't want the
> > kernel log in general to be security sensitive and restricted.
> > My goal here is not to push site-specific policy into the kernel but make life
> > easier for kernel developers by removing the confusing and error-prone %pK
> > altogether.
> 
>  Let me ask a different question then: is your approach to bulk-switch all 
> instances of `%pK' to `%p' as the safe default and let other people figure 
> out afterwards whether a different conversion specifier ought to be used 
> instead on a case-by-case basis and then follow up with another patch, or 
> will you consider these alternatives right away?

I am considering on a case-by-case basis. But mostly the decision is that %p is
enough, because by default %pK has been the same as %p anyways.
Also the current wave of replacements does not touch valid users of %pK.
They will stay and later be replaced with a new and better API.

> > Security is only one aspect.
> 
>  I think it's important enough though for us to ensure we don't compromise 
> it by chance.

Agreed.

Powered by blists - more mailing lists