Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
To:     Anssi Hannula <anssi.hannula@bitwise.fi>,
        Mathias Nyman <mathias.nyman@intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220222134117.34844-1-anssi.hannula@bitwise.fi>
From:   Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: Re: [PATCH 1/2] xhci: fix garbage USBSTS being logged in some cases
Message-ID: <fe7381b1-19bc-3b1e-50f3-0ed5c7c39e5e@linux.intel.com>
Date:   Wed, 23 Feb 2022 10:56:14 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20220222134117.34844-1-anssi.hannula@bitwise.fi>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 <anssi.hannula@bitwise.fi>
> ---
> 
> 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