linux-kernel - Re: [PATCH] usbip: fix vhci races in connection tear down

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 for Android: free password hash cracker in your pocket

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

Message-ID: <b0891cea-579f-2c29-4f3e-52713b836dce@linuxfoundation.org>
Date:   Fri, 12 Mar 2021 13:42:26 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Hillf Danton <hdanton@...a.com>
Cc:     valentina.manea.m@...il.com, shuah@...nel.org,
        gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot+a93fba6d384346a761e3@...kaller.appspotmail.com,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] usbip: fix vhci races in connection tear down

On 3/12/21 12:08 AM, Hillf Danton wrote:
> On Thu, 11 Mar 2021 19:27:37 -0700  Shuah Khan wrote:
>> vhci_shutdown_connection() references connection state (tcp_socket,
>> tcp_rx, tcp_tx, sockfd) saved in usbpip_device without holding the
>> lock.
>>
>> Current connection tear down sequence:
>> Step 1: shutdown the socket
>> Step 2: stop rx thread and reset tcp_rx pointer
>> Step 3: stop tx thread and reset tcp_tx pointer
>> Step 4: Reset tcp_socket and sockfd
>>
>> There are several race windows between these steps. In addition, device
>> reset routine (vhci_device_reset) resets tcp_socket and sockfd holding
>> the lock.
> 
> Can you specify the scenario where reset runs in race with teardown as
> both are parts of usbip_work on a singlethread workqueue?
>>

Hmm. I can't think of one. I was concerned about any async paths that
potentially interfere with shutdown. With vhci_shutdown_connection()
being so relaxed with locking, this is a cautious approach on my part.
I am also keeping in mind that this problem shows up in a limited
scope fuzzing test that doesn't trigger any other normal paths that
would be active if there is real device on the other side.

As for the tcp_socket check in the reset routine, I am not positive
what purpose it serves. I introduced the in_disconnect flag so
shutdown and reset don't collide, in case I am missing some scenario
in the normal path when we actually have a actual device attached.

With the other locking and error path problems in addressed, both
shutdown and reset could be made simpler.

In any case, I think in_disconnect might be too big a hammer. I will
redo the patch without it and also remove tcp_socket handling from
the reset routine. I don't see USBIP_EH_RESET getting set without
USBIP_EH_SHUTDOWN.

thanks,
-- Shuah