Messages in this thread | | | Subject | Re: Possible null pointer dereference in adutux.ko | From | Anton Volkov <> | Date | Fri, 18 Aug 2017 18:04:59 +0300 |
| |
On 15.08.2017 18:58, Oliver Neukum wrote: > Am Dienstag, den 15.08.2017, 16:38 +0300 schrieb Anton Volkov: >> On 15.08.2017 16:20, Oliver Neukum wrote: >>> >>> Am Dienstag, den 15.08.2017, 15:59 +0300 schrieb Anton Volkov: >>>> >>>> Hello. >>>> >>>> While searching for races in the Linux kernel I've come across >>>> "drivers/usb/misc/adutux.ko" module. Here is a question that I came up >>>> with while analyzing results. Lines are given using the info from Linux >>>> v4.12. >>>> >>>> Consider the following case: >>>> >>>> Thread 1: Thread 2: >>>> adu_release >>>> ->adu_release_internal adu_disconnect >>>> <READ &dev->udev->dev> dev->udev = NULL >>>> (adutux.c: line 298) (adutux.c: line 771) >>>> usb_deregister_dev >>>> >>>> Comments in the source code point at the possibility of adu_release() >>>> being called separately from adu_disconnect(). adu_release() and >>>> adu_disconnect() acquire different mutexes, so they are not protected >>>> from one another. If adu_disconnect() changes dev->udev before its value >>>> is read in adu_release_internal() there will be a NULL pointer >>>> dereference on a read attempt. Is this case feasible from your point of >>>> view? >>>> >>>> Thank you for your time. >>> >>> Hi, >>> >>> your analysis seems correct to me. In fact it looks like >>> >>> 66d4bc30d128e7c7ac4cf64aa78cb76e971cec5b >>> USB: adutux: remove custom debug macro >>> >>> more or less broke disconnect on this driver >>> (the URBs can also finish after dev->udev = NULL) >>> >>> Do you want to do a fix or do you want me to do it? >>> >>> Regards >>> Oliver >>> >> >> Hello, Oliver. >> >> I am not sure about the best way to solve this problem. If you have any >> ideas about it then it would probably be better if you could handle the >> fix. Or if you share the ideas I can prepare a patch. > > Hi, > > given the age of the drivers I would suggest to simply remove the debugging statements > > Regards > Oliver >
Hello, Oliver.
Looks like deletion of lots of debug print won't solve the race problem because there are other places that could potentially try to dereference dev->udev when disconnect has already poisoned it. For example in adu_open there are calls to usb_fill_int_urb with dev->udev as a parameter to be dereferenced inside the function.
There are other possible solutions, if I understand correctly: 1) although it is described that adutux_mutex should be used to protect only open_count, it usually protects the whole body of a function, so we could probably place it before the locking of dev->mtx; 2) move poisoning of dev->udev after usb_deregister_dev in order to wait for all other callbacks to finish.
What do you think?
Regards, Anton
-- Anton Volkov Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: avolkov@ispras.ru
| |