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.
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: [email protected]
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
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.
Regards,
Anton
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: [email protected]
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
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: [email protected]
Am Freitag, den 18.08.2017, 18:04 +0300 schrieb Anton Volkov:
>
> 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.
Yes, you are right.
> 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;
It seems to me that disconnect, open and release must take both
mutexes.
> 2) move poisoning of dev->udev after usb_deregister_dev in order to wait
> for all other callbacks to finish.
That would defeat the purpose of poisoning.
Regards
Oliver