Possible double free in iowarrior.ko
Hello.
While searching for races in the Linux kernel I've come across
"drivers/usb/misc/iowarrior.ko" module. Here are questions 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:
iowarrior_releaseiowarrior_disconnect
mutex_lock(>mutex)
dev->present = 0
(iowarrior.c: line 889)
mutex_lock(>mutex) mutex_unlock(>mutex)
dev->opened = 0
(iowarrior.c: line 666) if(dev->opened){
if(dev->present){ //dev->opened == 0
//dev->present ==0
} else { } else {
mutex_unlock(>mutex)iowarrior_delete(dev)
iowarrior_delete(dev) }
}
In this case double free of several pointers inside iowarrior_delete
becomes possible and no calls to usb_kill_urb() and
wake_up_interruptible() are present. Is this feasible from your point of
view? If so, maybe it is a good idea to move mutex_unlock(>mutex)
in iowarrior_disconnect() further down like in iowarrior_release() in
both 'if' branches?
Thank you for your time
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Possible bug in cypress_m8.ko
Hello.
Judging by the code of cypress_m8.c some functions are considered to be
capable of working concurrently with other functions, e.g. cypress_open.
There are, however, entities that are protected by the locks at one
place and not protected in another. Lines are given using the info from
Linux kernel v4.12. Example:
cypress_send
spin_lock_irqsave
priv->write_urb_in_use = 1;
spin_lock_irqrestore
(cypress_m8.c: lines 761-763)
...
if (result) {
priv->write_urb_in_use = 0; //without lock protection
(cypress_m8.c: line 783)
}
Is it a bug?
Thank you for your time.
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible null pointer dereference in adutux.ko
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 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: avol...@ispras.ru -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible null pointer dereference in adutux.ko
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 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: avol...@ispras.ru -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Possible null pointer dereference in adutux.ko
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 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: avol...@ispras.ru -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Possible null pointer dereference in isp1760.ko
Hello.
While searching for races in the Linux kernel I've come across
"drivers/usb/isp1760/isp1760.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:
isp1760_plat_probe
-> isp1760_register
-> isp1760_udc_register
request_irq(...,udc)
-> isp1760_udc_init_eps(udc)isp1760_udc_irq
for(i = 0; ...){ for(i = 0; ...) {
ep = >ep[i] ep = >ep[i%2]
-> isp1760_ep_rx_ready(ep)
INIT_LIST_HEAD(ep->queue) list_empty(>queue)
(isp1760-udc.c: line 1367)(isp1760-udc.c: line 303)
As far as I understand ep->queue is NULL before its initialization in
isp1760_udc_init_eps and list_empty() tries to access the '->next'
pointer member of the passed parameter.
Is this case possible from your point of view?
Thank you for your time.
-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
