Re: [PATCH 1/2] usb: core: Don't use %pK through printk

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Feb 17, 2025 at 03:50:54PM +0100, Thomas Weißschuh wrote:
> On Mon, Feb 17, 2025 at 02:52:05PM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Feb 17, 2025 at 02:20:51PM +0100, Thomas Weißschuh wrote:
> > > Restricted pointers ("%pK") are not meant to be used through printk().
> > > It can unintentionally expose security sensitive, raw pointer values.
> > > 
> > > Use regular pointer formatting instead.
> > > 
> > > Link: https://lore.kernel.org/lkml/20250113171731-dc10e3c1-da64-4af0-b767-7c7070468023@linutronix.de/
> > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > 
> > So really this is just a revert of 2f964780c03b ("USB: core: replace %p
> > with %pK"), right?
> 
> In this case, yes.

Great!  Mark it as such then please :)

> > Why not express it that way, and explain _why_ it's somehow now ok to
> > use %p when previously it wasn't?
> 
> The full background is in the email linked from the commit message.

That's not obvious at all when reviewing patches.  Please provide enough
information in the text itself to understand what is going on.  We
don't always have access to external links so we can't require them for
context.

> %p is more secure than %pK since
> commit ad67b74d2469 ("printk: hash addresses printed with %p").
> %pK was never intended to be used through printk() in the first place.

Great, say that then please.

> I'm doing the these changes for various subsystems using a common
> commit message. The changes are not reverts for all of them and
> digging out the specific history for each single line is a bunch
> of extra work.

Writing a good changelog is hard.  Trying to automate it like this is
going to be harder.  Just take the time to either do a revert (and
explain why), or do the change (and explain why).  Either way you have
to explain it properly, no shortcuts there.

> If you want more historical context, I'll resend the series, though.

As you are reverting a commit that was stated to be "for security", yes,
it better be redone, otherwise this is going to seem like a regression.

thanks,

greg k-h