linux-kernel - Re: [PATCH v2] 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: <118041cf-07b1-457c-ad59-b9c8d48342b9@linux.intel.com>
Date: Tue, 22 Oct 2024 17:33:37 +0300
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Faisal Hassan <quic_faisalh@...cinc.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Mathias Nyman <mathias.nyman@...el.com>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH v2] xhci: Fix Link TRB DMA in command ring stopped
 completion event

On 22.10.2024 15.34, Faisal Hassan wrote:
> Hi Mathias,
> 
>> Do we in this COMP_COMMAND_RING_STOPPED case even need to check if
>> cmd_dma != (u64)cmd_dequeue_dma, or if command ring stopped on a link TRB?
>>
>> Could we just move the COMP_COMMAND_RING_STOPPED handling a bit earlier?
>>
>> if (cmd_comp_code == COMP_COMMAND_RING_STOPPED) {
>>      complete_all(&xhci->cmd_ring_stop_completion);
>>          return;
>> }
>>
>> If I remember correctly it should just turn aborted command TRBs into
>> no-ops,
>> and restart the command ring
>>
> 
> Thanks for reviewing the changes!
> 
> Yes, you’re right. As part of restarting the command ring, we just ring
> the doorbell.
> 
> If we move the event handling without validating the dequeue pointer,
> wouldn’t it be a risk if we don’t check what the xHC is holding in its
> dequeue pointer? If we are not setting it, it starts from wherever it
> stopped. What if the dequeue pointer got corrupted or is not pointing to
> any of the TRBs in the command ring?

For that to happen the xHC host would have to corrupt its internal command
ring dequeue pointer. Not impossible, but an unlikely HW flaw, and a separate
issue. A case like that could be solved by writing the address of the next valid
(non-aborted) command to the CRCR register in xhci_handle_stopped_cmd_ring() before
ringing the doorbell.

The case you found where a command abort is not handled properly due to stopping
on a link TRB is a real xhci driver issue that would be nice to get solved.

For the COMP_COMMAND_RING_STOPPED case we don't really care that much
on which command it stopped, for other commands we do.

Thanks
Mathias