| Message ID | 20250217-restricted-pointers-usb-v1-1-78da55158832@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: Don't use %pK through printk | expand |
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? Why not express it that way, and explain _why_ it's somehow now ok to use %p when previously it wasn't? thanks, greg k-h
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. > 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. %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. 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. If you want more historical context, I'll resend the series, though. Thomas
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
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index a75cf1f6d741cf5827b9c4deca3b63013aff6cfe..46026b331267ade29839393b2fb5c0c42e34ab84 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1609,7 +1609,7 @@ int usb_hcd_unlink_urb (struct urb *urb, int status) if (retval == 0) retval = -EINPROGRESS; else if (retval != -EIDRM && retval != -EBUSY) - dev_dbg(&udev->dev, "hcd_unlink_urb %pK fail %d\n", + dev_dbg(&udev->dev, "hcd_unlink_urb %p fail %d\n", urb, retval); usb_put_dev(udev); } @@ -1786,7 +1786,7 @@ void usb_hcd_flush_endpoint(struct usb_device *udev, /* kick hcd */ unlink1(hcd, urb, -ESHUTDOWN); dev_dbg (hcd->self.controller, - "shutdown urb %pK ep%d%s-%s\n", + "shutdown urb %p ep%d%s-%s\n", urb, usb_endpoint_num(&ep->desc), is_in ? "in" : "out", usb_ep_type_string(usb_endpoint_type(&ep->desc))); diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 7576920e2d5a3e6c0dfd8bee8fce9d09a55c195c..5e52a35486afbe58bdffd3dfc1eb5964a9471ade 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -376,7 +376,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) if (!urb || !urb->complete) return -EINVAL; if (urb->hcpriv) { - WARN_ONCE(1, "URB %pK submitted while active\n", urb); + WARN_ONCE(1, "URB %p submitted while active\n", urb); return -EBUSY; }
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> --- drivers/usb/core/hcd.c | 4 ++-- drivers/usb/core/urb.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)