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] [day] [month] [year] [list]

Message-ID: <e64f1487-b3a8-4035-90f8-0062649996fd@quicinc.com>
Date: Sat, 19 Oct 2024 23:22:33 +0530
From: Faisal Hassan <quic_faisalh@...cinc.com>
To: Michał Pecio <michal.pecio@...il.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 Michal,

On 10/19/2024 12:50 PM, Michał Pecio wrote:
> 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
> 

Sure, this looks good. Let me revise the patch.

> 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

Thank you for the suggestions. I will submit a v2 version.

Regards,
Faisal