linux-kernel - [PATCH 2/3] usb: dwc2: Abort transaction after errors with unknown reason

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20210113112052.17063-3-nsaenzjulienne@suse.de>
Date:   Wed, 13 Jan 2021 12:20:50 +0100
From:   Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
To:     Paul Zimmerman <Paul.Zimmerman@...opsys.com>,
        Felipe Balbi <balbi@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Nick Hudson <skrll@...bsd.org>, linux-usb@...r.kernel.org,
        Minas Harutyunyan <hminas@...opsys.com>,
        Paul Zimmerman <paulz@...opsys.com>
Cc:     linux@...ck-us.net, dianders@...omium.org,
        Boris ARZUR <boris@...bu.org>,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        linux-kernel@...r.kernel.org
Subject: [PATCH 2/3] usb: dwc2: Abort transaction after errors with unknown reason

From: Guenter Roeck <linux@...ck-us.net>

In some situations, the following error messages are reported.

dwc2 ff540000.usb: dwc2_hc_chhltd_intr_dma: Channel 1 - ChHltd set, but reason is unknown
dwc2 ff540000.usb: hcint 0x00000002, intsts 0x04000021

This is sometimes followed by:

dwc2 ff540000.usb: dwc2_update_urb_state_abn(): trimming xfer length

and then:

WARNING: CPU: 0 PID: 0 at kernel/v4.19/drivers/usb/dwc2/hcd.c:2913
			dwc2_assign_and_init_hc+0x98c/0x990

The warning suggests that an odd buffer address is to be used for DMA.

After an error is observed, the receive buffer may be full
(urb->actual_length >= urb->length). However, the urb is still left in
the queue unless three errors were observed in a row. When it is queued
again, the dwc2 hcd code translates this into a 1-block transfer.
If urb->actual_length (ie the total expected receive length) is not
DMA-aligned, the buffer pointer programmed into the chip will be
unaligned. This results in the observed warning.

To solve the problem, abort input transactions after an error with
unknown cause if the entire packet was already received. This may be
a bit drastic, but we don't really know why the transfer was aborted
even though the entire packet was received. Aborting the transfer in
this situation is less risky than accepting a potentially corrupted
packet.

With this patch in place, the 'ChHltd set' and 'trimming xfer length'
messages are still observed, but there are no more transfer attempts
with odd buffer addresses.

Cc: Boris ARZUR <boris@...bu.org>
Cc: Douglas Anderson <dianders@...omium.org>
Signed-off-by: Guenter Roeck <linux@...ck-us.net>
Reviewed-by: Douglas Anderson <dianders@...omium.org>
Fixes: 151d0cbdbe860 ("usb: dwc2: make the scheduler handle excessive NAKs better")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
---
 drivers/usb/dwc2/hcd_intr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index a052d39b4375..12819e019e13 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1977,6 +1977,18 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg *hsotg,
 		qtd->error_count++;
 		dwc2_update_urb_state_abn(hsotg, chan, chnum, qtd->urb,
 					  qtd, DWC2_HC_XFER_XACT_ERR);
+		/*
+		 * We can get here after a completed transaction
+		 * (urb->actual_length >= urb->length) which was not reported
+		 * as completed. If that is the case, and we do not abort
+		 * the transfer, a transfer of size 0 will be enqueued
+		 * subsequently. If urb->actual_length is not DMA-aligned,
+		 * the buffer will then point to an unaligned address, and
+		 * the resulting behavior is undefined. Bail out in that
+		 * situation.
+		 */
+		if (qtd->urb->actual_length >= qtd->urb->length)
+			qtd->error_count = 3;
 		dwc2_hcd_save_data_toggle(hsotg, chan, chnum, qtd);
 		dwc2_halt_channel(hsotg, chan, qtd, DWC2_HC_XFER_XACT_ERR);
 	}
-- 
2.29.2