Possible null pointer dereference in adutux.ko

Possible null pointer dereference in adutux.ko

[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.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avolkov@ispras.ru

Re: Possible null pointer dereference in adutux.ko

[Oliver Neukum]

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

Re: Possible null pointer dereference in adutux.ko

[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.

Regards,
Anton

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avolkov@ispras.ru

Re: Possible null pointer dereference in adutux.ko

[Oliver Neukum]

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

Re: Possible null pointer dereference in adutux.ko

[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.

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

Re: Possible null pointer dereference in adutux.ko

[Oliver Neukum]

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