Bug in stv680

Bug in stv680

[David Ellingsworth]

I'm currently in the process of developing driver for a currently
unsupported usb camera. For guidance, I've been referencing the code
of other usb camera drivers. While reviewing the stv680 code, I've
come across a bug that looks like it could crash the driver. Since I
do not have a camera that is supported by this driver, I can only rely
on my interpretation of the code and how my still underdeveloped
driver behaves.

In stv_open, stv680->user is set to 1. In stv_close, stv680->user is
set to 0. In stv680_disconnect, the value of stv680->user is used to
determine if the stv680 struct should be freed. Under the following
conditions stv680->user will be 0 and the stv680 struct will be freed
while it is still in use.
     1. Application 1 opens the device. stv680->user transitions from 0->1
     2. Application 2 opens the device. stv680->user transitions from 1->1
     3. Application 2 closes the device. stv680->user transitions from 1->0
     4. Device is physically disconnected. stv680 is freed.
     5. Application 1 closes the device.

The crash could happen at step 4 in stv680_read, stv680_mmap, or
stv680_do_ioctl, or at step 5 in stv_close depending on how
Application 1 is using the device.

The apparent fix for this, is that stv680->user should be incremented
whenever the device is opened, and decremented when it is closed.
Likewise, a lock should be used to guarantee exclusive access to
stv680->user to avoid a race condition between stv_open and stv_close.
This should correct the case where the structure is freed by
stv_disconnect while it is still open and in use.

I should note, the v4l2 specs indicate that the device may be opened
multiple times as in the case above. The reasoning for this is that
while one application is streaming the video, another may modify
controls. A valid fix would therefore not be to deny multiple opens to
the device.

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Bug in stv680

[Alan Cox]

On Wed, Jun 25, 2008 at 03:51:33PM -0400, David Ellingsworth wrote:
>      1. Application 1 opens the device. stv680->user transitions from 0->1
>      2. Application 2 opens the device. stv680->user transitions from 1->1
>      3. Application 2 closes the device. stv680->user transitions from 1->0
>      4. Device is physically disconnected. stv680 is freed.
>      5. Application 1 closes the device.
> 
> The crash could happen at step 4 in stv680_read, stv680_mmap, or
> stv680_do_ioctl, or at step 5 in stv_close depending on how
> Application 1 is using the device.

Yes I think you are right

> The apparent fix for this, is that stv680->user should be incremented
> whenever the device is opened, and decremented when it is closed.
> Likewise, a lock should be used to guarantee exclusive access to
> stv680->user to avoid a race condition between stv_open and stv_close.
> This should correct the case where the structure is freed by
> stv_disconnect while it is still open and in use.

Currently open/close use the big kernel lock so the locking part is ok. The
'proper' kernel way to do this is to use krefs which basically implement
the locking you describe but using atomic_inc/dec and atomic decrement and
compare with zero operations from the various architecture layers.


Something like

struct stv680 *stv_get(struct stv680 *v)
{
	kref_get(&v->kref);
}

void stv_put(struct stv680 *)
{
	kref_put(&v->kref, stv_free_thingies);
}


One camera driver that does use krefs properly (they are fairly new so
a lot of drivers don't yet adopt them) is the stk-webcam driver which uses
them for open/close as you describe.


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Bug in stv680

[David Ellingsworth]

On Thu, Jun 26, 2008 at 6:13 AM, Alan Cox <alan@redhat.com> wrote:
> On Wed, Jun 25, 2008 at 03:51:33PM -0400, David Ellingsworth wrote:
>>      1. Application 1 opens the device. stv680->user transitions from 0->1
>>      2. Application 2 opens the device. stv680->user transitions from 1->1
>>      3. Application 2 closes the device. stv680->user transitions from 1->0
>>      4. Device is physically disconnected. stv680 is freed.
>>      5. Application 1 closes the device.
>>
>> The crash could happen at step 4 in stv680_read, stv680_mmap, or
>> stv680_do_ioctl, or at step 5 in stv_close depending on how
>> Application 1 is using the device.
>
> Yes I think you are right
>
>> The apparent fix for this, is that stv680->user should be incremented
>> whenever the device is opened, and decremented when it is closed.
>> Likewise, a lock should be used to guarantee exclusive access to
>> stv680->user to avoid a race condition between stv_open and stv_close.
>> This should correct the case where the structure is freed by
>> stv_disconnect while it is still open and in use.
>
> Currently open/close use the big kernel lock so the locking part is ok. The
> 'proper' kernel way to do this is to use krefs which basically implement
> the locking you describe but using atomic_inc/dec and atomic decrement and
> compare with zero operations from the various architecture layers.
>
>
> Something like
>
> struct stv680 *stv_get(struct stv680 *v)
> {
>        kref_get(&v->kref);
> }
>
> void stv_put(struct stv680 *)
> {
>        kref_put(&v->kref, stv_free_thingies);
> }
>
>
> One camera driver that does use krefs properly (they are fairly new so
> a lot of drivers don't yet adopt them) is the stk-webcam driver which uses
> them for open/close as you describe.
>
>
>
Thanks for clarifying the locking structure. Somehow I missed the
comment about the BKL being held. None the less, the scenario I
provided should result in a crash due an invalid internal reference
count.

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list