linux-kernel - Re: [PATCH] xhci: Fix Link TRB DMA in command ring stopped completion event

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: <20241019092023.5d987d7e@foxbook>
Date: Sat, 19 Oct 2024 09:20:23 +0200
From: Michał Pecio <michal.pecio@...il.com>
To: quic_faisalh@...cinc.com
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
 linux-usb@...r.kernel.org, mathias.nyman@...el.com, stable@...r.kernel.org
Subject: Re: [PATCH] xhci: Fix Link TRB DMA in command ring stopped
 completion event

Hi,

> During the aborting of a command, the software receives a command
> completion event for the command ring stopped, with the TRB pointing
> to the next TRB after the aborted command.
>
> If the command we abort is located just before the Link TRB in the
> command ring, then during the 'command ring stopped' completion event,
> the xHC gives the Link TRB in the event's cmd DMA, which causes a
> mismatch in handling command completion event.
>
> To handle this situation, an additional check has been added to ignore
> the mismatch error and continue the operation.

Thanks, I remember having some issues with command aborts, but I blamed
them on my own bugs, although I never found what the problem was. I may
take a look at it later, but I'm currently busy with other things.

No comment about validity of this patch for now, but a few remarks:

>+static bool is_dma_link_trb(struct xhci_ring *ring, dma_addr_t dma)
>+{
>+	struct xhci_segment *seg;
>+	union xhci_trb *trb;
>+	dma_addr_t trb_dma;
>+	int i;
>+
>+	seg = ring->first_seg;
>+	do {
>+		for (i = 0; i < TRBS_PER_SEGMENT; i++) {
>+			trb = &seg->trbs[i];
>+			trb_dma = seg->dma + (i * sizeof(union xhci_trb));
>+
>+			if (trb_is_link(trb) && trb_dma == dma)
>+				return true;
>+		}

You don't need to iterate the array. Something like this should work:
do {
	if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE)) {
		/* found the TRB, check if it's link */
		trb = &seg->trbs[(dma - seg->dma) / sizeof(*trb)];
		return trb_is_link(trb);
	}
	// try next seg, while (blah blah), return false

We should probably have a helper for (ring, dma)->trb lookups, but
for stable it may be sensible to do it without excess complication.

>+	if ((!cmd_dequeue_dma || cmd_dma != (u64)cmd_dequeue_dma) &&
>+	    !(cmd_comp_code == COMP_COMMAND_RING_STOPPED &&
>+	      is_dma_link_trb(xhci->cmd_ring, cmd_dma))) {

This if statement is quite complex now. I would be tempted to write
it this way instead:

/* original comment */
if (cmd_dma != dequeue_dma) {
	/* your new comment */
	if (! (RING_STOPPED && is_link)) {
		warn();
		return;
	}
}

Regards,
Michal