[PATCH 0/2] usb: Don't use %pK through printk

[PATCH 0/2] usb: Don't use %pK through printk

[Thomas Weißschuh]

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>
---
Thomas Weißschuh (2):
      usb: core: Don't use %pK through printk
      usb: dwc3: Don't use %pK through printk

 drivers/usb/core/hcd.c     | 4 ++--
 drivers/usb/core/urb.c     | 2 +-
 drivers/usb/dwc3/dwc3-st.c | 2 +-
 drivers/usb/dwc3/gadget.c  | 6 +++---
 4 files changed, 7 insertions(+), 7 deletions(-)
---
base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
change-id: 20250217-restricted-pointers-usb-8c711bae6994

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

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

[Thomas Weißschuh]

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(-)

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;
 	}
 

-- 
2.48.1

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

[Greg Kroah-Hartman]

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

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

[Thomas Weißschuh]

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

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

[Greg Kroah-Hartman]

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

[PATCH 2/2] usb: dwc3: Don't use %pK through printk

[Thomas Weißschuh]

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/dwc3/dwc3-st.c | 2 +-
 drivers/usb/dwc3/gadget.c  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
index ef7c43008946e15b72d88aba4941dc52bf0788d7..5d513decaacd22de15825dc061c2747cf09fef07 100644
--- a/drivers/usb/dwc3/dwc3-st.c
+++ b/drivers/usb/dwc3/dwc3-st.c
@@ -225,7 +225,7 @@ static int st_dwc3_probe(struct platform_device *pdev)
 
 	dwc3_data->syscfg_reg_off = res->start;
 
-	dev_vdbg(dev, "glue-logic addr 0x%pK, syscfg-reg offset 0x%x\n",
+	dev_vdbg(dev, "glue-logic addr 0x%p, syscfg-reg offset 0x%x\n",
 		 dwc3_data->glue_base, dwc3_data->syscfg_reg_off);
 
 	struct device_node *child __free(device_node) = of_get_compatible_child(node,
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ddd6b2ce57107cb62c5cec02eae3fe3524164469..7ea1dcd07f02999c913f1520ec05bbcf1a43d8d5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1971,12 +1971,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 		return -ESHUTDOWN;
 	}
 
-	if (WARN(req->dep != dep, "request %pK belongs to '%s'\n",
+	if (WARN(req->dep != dep, "request %p belongs to '%s'\n",
 				&req->request, req->dep->name))
 		return -EINVAL;
 
 	if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED,
-				"%s: request %pK already in flight\n",
+				"%s: request %p already in flight\n",
 				dep->name, &req->request))
 		return -EINVAL;
 
@@ -2165,7 +2165,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 		}
 	}
 
-	dev_err(dwc->dev, "request %pK was not queued to %s\n",
+	dev_err(dwc->dev, "request %p was not queued to %s\n",
 		request, ep->name);
 	ret = -EINVAL;
 out:

-- 
2.48.1

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

[Thinh Nguyen]

On Mon, Feb 17, 2025, 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://urldefense.com/v3/__https://lore.kernel.org/lkml/20250113171731-dc10e3c1-da64-4af0-b767-7c7070468023@linutronix.de/__;!!A4F2R9G_pg!aL6340JEjoiSy7grYBE9C7kitN3lR7BsH4zWSkY6QKPCrC_SDBPIL8rZaoIXxABAdIRsEC2YPI_AtCU9tMrD1byOXeZ93YpywfAFLw$ 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  drivers/usb/dwc3/dwc3-st.c | 2 +-
>  drivers/usb/dwc3/gadget.c  | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> index ef7c43008946e15b72d88aba4941dc52bf0788d7..5d513decaacd22de15825dc061c2747cf09fef07 100644
> --- a/drivers/usb/dwc3/dwc3-st.c
> +++ b/drivers/usb/dwc3/dwc3-st.c
> @@ -225,7 +225,7 @@ static int st_dwc3_probe(struct platform_device *pdev)
>  
>  	dwc3_data->syscfg_reg_off = res->start;
>  
> -	dev_vdbg(dev, "glue-logic addr 0x%pK, syscfg-reg offset 0x%x\n",
> +	dev_vdbg(dev, "glue-logic addr 0x%p, syscfg-reg offset 0x%x\n",
>  		 dwc3_data->glue_base, dwc3_data->syscfg_reg_off);
>  
>  	struct device_node *child __free(device_node) = of_get_compatible_child(node,
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index ddd6b2ce57107cb62c5cec02eae3fe3524164469..7ea1dcd07f02999c913f1520ec05bbcf1a43d8d5 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1971,12 +1971,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  		return -ESHUTDOWN;
>  	}
>  
> -	if (WARN(req->dep != dep, "request %pK belongs to '%s'\n",
> +	if (WARN(req->dep != dep, "request %p belongs to '%s'\n",
>  				&req->request, req->dep->name))
>  		return -EINVAL;
>  
>  	if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED,
> -				"%s: request %pK already in flight\n",
> +				"%s: request %p already in flight\n",
>  				dep->name, &req->request))
>  		return -EINVAL;
>  
> @@ -2165,7 +2165,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  		}
>  	}
>  
> -	dev_err(dwc->dev, "request %pK was not queued to %s\n",
> +	dev_err(dwc->dev, "request %p was not queued to %s\n",
>  		request, ep->name);
>  	ret = -EINVAL;
>  out:
> 
> -- 
> 2.48.1
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh