2022-02-22 15:53:30

by Anssi Hannula

[permalink] [raw]
Subject: [PATCH 1/2] xhci: fix garbage USBSTS being logged in some cases

xhci_decode_usbsts() is expected to return a zero-terminated string by
its only caller, xhci_stop_endpoint_command_watchdog(), which directly
logs the return value:

xhci_warn(xhci, "USBSTS:%s\n", xhci_decode_usbsts(str, usbsts));

However, if no recognized bits are set in usbsts, the function will
return without having called any sprintf() and therefore return an
untouched non-zero-terminated caller-provided buffer, causing garbage
to be output to log.

Fix that by always including the raw value in the output.

Note that before 4843b4b5ec64 ("xhci: fix even more unsafe memory usage
in xhci tracing") the result effect in the failure case was different as
a static buffer was used here, but the code still worked incorrectly.

Fixes: 9c1aa36efdae ("xhci: Show host status when watchdog triggers and host is assumed dead.")
Signed-off-by: Anssi Hannula <[email protected]>
---

Noticed this while debugging a USB issue. Let me know if you prefer a
different fix.

drivers/usb/host/xhci.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 8a0026ee9524..ac91647195f6 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2642,6 +2642,7 @@ static inline const char *xhci_decode_usbsts(char *str, u32 usbsts)
ret += sprintf(str + ret, " CNR");
if (usbsts & STS_HCE)
ret += sprintf(str + ret, " HCE");
+ ret += sprintf(str + ret, " (0x%08x)", usbsts);

return str;
}
--
2.34.1


2022-02-23 11:42:08

by Mathias Nyman

[permalink] [raw]
Subject: Re: [PATCH 1/2] xhci: fix garbage USBSTS being logged in some cases

On 22.2.2022 15.41, Anssi Hannula wrote:
> xhci_decode_usbsts() is expected to return a zero-terminated string by
> its only caller, xhci_stop_endpoint_command_watchdog(), which directly
> logs the return value:
>
> xhci_warn(xhci, "USBSTS:%s\n", xhci_decode_usbsts(str, usbsts));
>
> However, if no recognized bits are set in usbsts, the function will
> return without having called any sprintf() and therefore return an
> untouched non-zero-terminated caller-provided buffer, causing garbage
> to be output to log.
>
> Fix that by always including the raw value in the output.
>
> Note that before 4843b4b5ec64 ("xhci: fix even more unsafe memory usage
> in xhci tracing") the result effect in the failure case was different as
> a static buffer was used here, but the code still worked incorrectly.
>
> Fixes: 9c1aa36efdae ("xhci: Show host status when watchdog triggers and host is assumed dead.")
> Signed-off-by: Anssi Hannula <[email protected]>
> ---
>
> Noticed this while debugging a USB issue. Let me know if you prefer a
> different fix.
>
> drivers/usb/host/xhci.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 8a0026ee9524..ac91647195f6 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -2642,6 +2642,7 @@ static inline const char *xhci_decode_usbsts(char *str, u32 usbsts)
> ret += sprintf(str + ret, " CNR");
> if (usbsts & STS_HCE)
> ret += sprintf(str + ret, " HCE");
> + ret += sprintf(str + ret, " (0x%08x)", usbsts);

Thanks, nice catch.

Maybe this could be the first thing printed out, something like (untested):

@@ -2697,8 +2697,11 @@ static inline const char *xhci_decode_usbsts(char *str, u32 usbsts)
{
int ret = 0;

+ ret = sprintf(str, " 0x%08x", usbsts);
+
if (usbsts == ~(u32)0)
- return " 0xffffffff";
+ return str;
+

-Mathias

2022-02-25 13:05:30

by Anssi Hannula

[permalink] [raw]
Subject: [PATCH 1/2 v2] xhci: fix garbage USBSTS being logged in some cases

xhci_decode_usbsts() is expected to return a zero-terminated string by
its only caller, xhci_stop_endpoint_command_watchdog(), which directly
logs the return value:

xhci_warn(xhci, "USBSTS:%s\n", xhci_decode_usbsts(str, usbsts));

However, if no recognized bits are set in usbsts, the function will
return without having called any sprintf() and therefore return an
untouched non-zero-terminated caller-provided buffer, causing garbage
to be output to log.

Fix that by always including the raw value in the output.

Note that before 4843b4b5ec64 ("xhci: fix even more unsafe memory usage
in xhci tracing") the result effect in the failure case was different as
a static buffer was used here, but the code still worked incorrectly.

Fixes: 9c1aa36efdae ("xhci: Show host status when watchdog triggers and host is assumed dead.")
Signed-off-by: Anssi Hannula <[email protected]>
---

Mathias Nyman wrote:
> Maybe this could be the first thing printed out, something like (untested):
[...]

Heh, that's actually pretty close to what I had at one point, not sure
why I didn't go with it. I agree it looks better.

Changed and tested on real HW:

[ 11.998832] xhci-hcd xhci-hcd.1.auto: xHCI host not responding to stop endpoint command.
[ 12.006925] xhci-hcd xhci-hcd.1.auto: USBSTS: 0x00000000


v2: Improve print format on Mathias Nyman's suggestion.

drivers/usb/host/xhci.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 8a0026ee9524..dd24c09927bd 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2622,8 +2622,11 @@ static inline const char *xhci_decode_usbsts(char *str, u32 usbsts)
{
int ret = 0;

+ ret = sprintf(str, " 0x%08x", usbsts);
+
if (usbsts == ~(u32)0)
- return " 0xffffffff";
+ return str;
+
if (usbsts & STS_HALT)
ret += sprintf(str + ret, " HCHalted");
if (usbsts & STS_FATAL)
--
2.34.1

2022-02-28 13:20:14

by Mathias Nyman

[permalink] [raw]
Subject: Re: [PATCH 1/2 v2] xhci: fix garbage USBSTS being logged in some cases

On 25.2.2022 12.26, Anssi Hannula wrote:
> xhci_decode_usbsts() is expected to return a zero-terminated string by
> its only caller, xhci_stop_endpoint_command_watchdog(), which directly
> logs the return value:
>
> xhci_warn(xhci, "USBSTS:%s\n", xhci_decode_usbsts(str, usbsts));
>
> However, if no recognized bits are set in usbsts, the function will
> return without having called any sprintf() and therefore return an
> untouched non-zero-terminated caller-provided buffer, causing garbage
> to be output to log.
>
> Fix that by always including the raw value in the output.
>
> Note that before 4843b4b5ec64 ("xhci: fix even more unsafe memory usage
> in xhci tracing") the result effect in the failure case was different as
> a static buffer was used here, but the code still worked incorrectly.
>
> Fixes: 9c1aa36efdae ("xhci: Show host status when watchdog triggers and host is assumed dead.")
> Signed-off-by: Anssi Hannula <[email protected]>
> ---

Thanks. Adding both 1/2 v2 and 2/2

-Mathias