linux-kernel - Re: Fix locking in mousedev

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]

Date:	Fri, 9 Mar 2007 09:28:49 -0500
From:	"Dmitry Torokhov" <dmitry.torokhov@...il.com>
To:	"Pete Zaitcev" <zaitcev@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-input@...ey.karlin.mff.cuni.cz
Subject: Re: Fix locking in mousedev

Hi Pete,

On 2/28/07, Pete Zaitcev <zaitcev@...hat.com> wrote:
> If a process is closing /dev/input/mice and an mouse disconnects simulta-
> neously, the system is likely to oops. This usually happens when someone
> hits <Alt><Ctrl>F1 or logs out from X, and flips a KVM while the system
> is reacting.
>
> I reproduced the issue by running this:
>  while true; do cat </dev/null >/dev/input/mice; done
> This way, it oopses on 2nd or 3rd disconnect reliably. With the patch,
> I can disconnect the mouse 20 times.
>
> Signed-off-by: Pete Zaitcev <zaitcev@...hat.com>
>
> ---
>
> Discussion
>
> One of the race scenarios is related to the list of handles. The cat
> calls mousedev_close -> mixdev_release, does list_for_each to walk for
> all handles for a given handler. Iterations are longish while it does
> input_close_device -> hidinput_close -> usbhid_close -> usb_kill_urb,
> which sleeps briefly. Into this gap goes khubd and does hid_disconnect ->
> hidinput_disconnect -> input_unregister_device. This corrupts the list
> of handles which cat process is walking.
>
> I was unable to devise a scheme to protect the stock h_list adequately,
> so I implemented a private list of mousedev instances, which can be
> protected correctly.
>
> Dmitry, please consider getting rid of the list of handles entirely.
> The other major user is drivers/char/keyboard.c.
>

I agree that handlers should not access h_list nad use their own
private lists instead. However input core still needs that list to
maintain its books.

> Other than that, the patch is straightforward. It adds a static mutex
> to guard common data structures. It has to be static because instances
> of mousedev share common structures, such as the mousedev_table[].
>
> This should be uncontroversial, but please let me know if I missed
> something obvious.
>

I agree with the patch, unfortunately in lands squarely in the middle
of me restructuring the code (swiitch to struct device, proper
refcounting, etc) but I will try to adopt it.

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/