[RFC PATCH] hidraw: protect hidraw_disconnect() better

[RFC PATCH] hidraw: protect hidraw_disconnect() better

[James Hogan]

The following patch I think fixes a bug in hidraw_disconnect(). However I'm unsure whether it's safe to call device_destroy with the minors_lock held. can the device_destroy ever end up calling hidraw_release, resulting in recursive locking? I've never seen that happen, but I don't understand the inner workings on device_destroy.

The bug can be revealed with SLAB debugging on (poisoning free'd memory), and:
cat /dev/hw_random > /dev/hidraw0
then unplug the device. the disconnect is called, the device_destroy seems to cause "cat"'s write syscall to return a timeout error, so it exits/closes, which frees the hidraw because hidraw->exists==0, then the disconnect function writes to hidraw_table[hidraw->minor] which blows up because hidraw->minor has been poisoned with 0x6b6b6b6b.

This has been tested on 2.6.39 and appears to fix it, and I'll hopefully be able to test it on the latest kernel tonight.

Cheers
James

The function hidraw_disconnect() only acquires the hidraw minors_lock
when clearing the entry in hidraw_table. However the device_destroy()
call can cause a userland read/write to return with an error. It may
cause the program to release the file descripter before the disconnect
is finished. hidraw_disconnect() has already set hidraw->exist to 0,
which makes hidraw_release() kfree the hidraw structure, which
hidraw_disconnect() continues to access and even tries to kfree again.
Similarly if a hidraw_release() occurs after setting hidraw->exist to 0,
the same thing can happen.

This is fixed by expanding the mutex critical section to cover the whole
function from setting hidraw->exist to 0 to freeing the hidraw
structure, preventing a hidraw_release() from interfering.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/hid/hidraw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index c79578b..a8c2b7b 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -510,13 +510,12 @@ void hidraw_disconnect(struct hid_device *hid)
 {
 	struct hidraw *hidraw = hid->hidraw;
 
+	mutex_lock(&minors_lock);
 	hidraw->exist = 0;
 
 	device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
 
-	mutex_lock(&minors_lock);
 	hidraw_table[hidraw->minor] = NULL;
-	mutex_unlock(&minors_lock);
 
 	if (hidraw->open) {
 		hid_hw_close(hid);
@@ -524,6 +523,7 @@ void hidraw_disconnect(struct hid_device *hid)
 	} else {
 		kfree(hidraw);
 	}
+	mutex_unlock(&minors_lock);
 }
 EXPORT_SYMBOL_GPL(hidraw_disconnect);
 
-- 
1.7.2.3

Re: [RFC PATCH] hidraw: protect hidraw_disconnect() better

[James Hogan]

On Mon, Sep 19, 2011 at 03:42:34PM +0100, James Hogan wrote:
> The following patch I think fixes a bug in hidraw_disconnect(). However I'm unsure whether it's safe to call device_destroy with the minors_lock held. can the device_destroy ever end up calling hidraw_release, resulting in recursive locking? I've never seen that happen, but I don't understand the inner workings on device_destroy.
> 
> The bug can be revealed with SLAB debugging on (poisoning free'd memory), and:
> cat /dev/hw_random > /dev/hidraw0
> then unplug the device. the disconnect is called, the device_destroy seems to cause "cat"'s write syscall to return a timeout error, so it exits/closes, which frees the hidraw because hidraw->exists==0, then the disconnect function writes to hidraw_table[hidraw->minor] which blows up because hidraw->minor has been poisoned with 0x6b6b6b6b.
> 
> This has been tested on 2.6.39 and appears to fix it, and I'll hopefully be able to test it on the latest kernel tonight.

I have now tested this on a PC with slub_debug on the command line and
Linus' latest tree (3.1-rc6):
$ while [ -e /dev/hidraw0 ]; do echo hello; done > /dev/hidraw0
and unplugged the device.

Before the patch it caused a fault with callstack etc. the first time I
tried it. I tried 30 times with the patch and got no such errors. I
haven't tried anything more hidraw related though.

Cheers
James

> 
> Cheers
> James
> 
> The function hidraw_disconnect() only acquires the hidraw minors_lock
> when clearing the entry in hidraw_table. However the device_destroy()
> call can cause a userland read/write to return with an error. It may
> cause the program to release the file descripter before the disconnect
> is finished. hidraw_disconnect() has already set hidraw->exist to 0,
> which makes hidraw_release() kfree the hidraw structure, which
> hidraw_disconnect() continues to access and even tries to kfree again.
> Similarly if a hidraw_release() occurs after setting hidraw->exist to 0,
> the same thing can happen.
> 
> This is fixed by expanding the mutex critical section to cover the whole
> function from setting hidraw->exist to 0 to freeing the hidraw
> structure, preventing a hidraw_release() from interfering.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>  drivers/hid/hidraw.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index c79578b..a8c2b7b 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -510,13 +510,12 @@ void hidraw_disconnect(struct hid_device *hid)
>  {
>  	struct hidraw *hidraw = hid->hidraw;
>  
> +	mutex_lock(&minors_lock);
>  	hidraw->exist = 0;
>  
>  	device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
>  
> -	mutex_lock(&minors_lock);
>  	hidraw_table[hidraw->minor] = NULL;
> -	mutex_unlock(&minors_lock);
>  
>  	if (hidraw->open) {
>  		hid_hw_close(hid);
> @@ -524,6 +523,7 @@ void hidraw_disconnect(struct hid_device *hid)
>  	} else {
>  		kfree(hidraw);
>  	}
> +	mutex_unlock(&minors_lock);
>  }
>  EXPORT_SYMBOL_GPL(hidraw_disconnect);
>  
> -- 
> 1.7.2.3
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] hidraw: protect hidraw_disconnect() better

[David Herrmann]

Hi James

On Mon, Sep 19, 2011 at 4:42 PM, James Hogan <james.hogan@imgtec.com> wrote:
> The following patch I think fixes a bug in hidraw_disconnect(). However I'm unsure whether it's safe to call device_destroy with the minors_lock held. can the device_destroy ever end up calling hidraw_release, resulting in recursive locking? I've never seen that happen, but I don't understand the inner workings on device_destroy.
>
> The bug can be revealed with SLAB debugging on (poisoning free'd memory), and:
> cat /dev/hw_random > /dev/hidraw0
> then unplug the device. the disconnect is called, the device_destroy seems to cause "cat"'s write syscall to return a timeout error, so it exits/closes, which frees the hidraw because hidraw->exists==0, then the disconnect function writes to hidraw_table[hidraw->minor] which blows up because hidraw->minor has been poisoned with 0x6b6b6b6b.
>
> This has been tested on 2.6.39 and appears to fix it, and I'll hopefully be able to test it on the latest kernel tonight.
>
> Cheers
> James
>
> The function hidraw_disconnect() only acquires the hidraw minors_lock
> when clearing the entry in hidraw_table. However the device_destroy()
> call can cause a userland read/write to return with an error. It may
> cause the program to release the file descripter before the disconnect
> is finished. hidraw_disconnect() has already set hidraw->exist to 0,
> which makes hidraw_release() kfree the hidraw structure, which
> hidraw_disconnect() continues to access and even tries to kfree again.
> Similarly if a hidraw_release() occurs after setting hidraw->exist to 0,
> the same thing can happen.
>
> This is fixed by expanding the mutex critical section to cover the whole
> function from setting hidraw->exist to 0 to freeing the hidraw
> structure, preventing a hidraw_release() from interfering.
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>  drivers/hid/hidraw.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index c79578b..a8c2b7b 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -510,13 +510,12 @@ void hidraw_disconnect(struct hid_device *hid)
>  {
>        struct hidraw *hidraw = hid->hidraw;
>
> +       mutex_lock(&minors_lock);
>        hidraw->exist = 0;
>
>        device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));

This does not destroy any open file descriptor and we haven't
registered any kind of hook so hidraw_destroy() will not be called
here.
This seems safe to me.
We also do not check for hidraw->exist on *_open() callback so
including this in the critical section seems fine.

> -       mutex_lock(&minors_lock);
>        hidraw_table[hidraw->minor] = NULL;
> -       mutex_unlock(&minors_lock);
>
>        if (hidraw->open) {
>                hid_hw_close(hid);
> @@ -524,6 +523,7 @@ void hidraw_disconnect(struct hid_device *hid)
>        } else {
>                kfree(hidraw);
>        }
> +       mutex_unlock(&minors_lock);
>  }
>  EXPORT_SYMBOL_GPL(hidraw_disconnect);
>
> --
> 1.7.2.3

Nice catch. I've tested it on linux-next tree and I can confirm the
bug. The fix seems ok to me.

Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] hidraw: protect hidraw_disconnect() better

[James Hogan]

Hi

On 09/20/2011 09:36 AM, David Herrmann wrote:
> Hi James
> 
> On Mon, Sep 19, 2011 at 4:42 PM, James Hogan <james.hogan@imgtec.com> wrote:
>> The following patch I think fixes a bug in hidraw_disconnect(). However I'm unsure whether it's safe to call device_destroy with the minors_lock held. can the device_destroy ever end up calling hidraw_release, resulting in recursive locking? I've never seen that happen, but I don't understand the inner workings on device_destroy.
>>
>> The bug can be revealed with SLAB debugging on (poisoning free'd memory), and:
>> cat /dev/hw_random > /dev/hidraw0
>> then unplug the device. the disconnect is called, the device_destroy seems to cause "cat"'s write syscall to return a timeout error, so it exits/closes, which frees the hidraw because hidraw->exists==0, then the disconnect function writes to hidraw_table[hidraw->minor] which blows up because hidraw->minor has been poisoned with 0x6b6b6b6b.
>>
>> This has been tested on 2.6.39 and appears to fix it, and I'll hopefully be able to test it on the latest kernel tonight.
>>
>> Cheers
>> James
>>
>> The function hidraw_disconnect() only acquires the hidraw minors_lock
>> when clearing the entry in hidraw_table. However the device_destroy()
>> call can cause a userland read/write to return with an error. It may
>> cause the program to release the file descripter before the disconnect
>> is finished. hidraw_disconnect() has already set hidraw->exist to 0,
>> which makes hidraw_release() kfree the hidraw structure, which
>> hidraw_disconnect() continues to access and even tries to kfree again.
>> Similarly if a hidraw_release() occurs after setting hidraw->exist to 0,
>> the same thing can happen.
>>
>> This is fixed by expanding the mutex critical section to cover the whole
>> function from setting hidraw->exist to 0 to freeing the hidraw
>> structure, preventing a hidraw_release() from interfering.
>>
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>> ---
>>  drivers/hid/hidraw.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
>> index c79578b..a8c2b7b 100644
>> --- a/drivers/hid/hidraw.c
>> +++ b/drivers/hid/hidraw.c
>> @@ -510,13 +510,12 @@ void hidraw_disconnect(struct hid_device *hid)
>>  {
>>        struct hidraw *hidraw = hid->hidraw;
>>
>> +       mutex_lock(&minors_lock);
>>        hidraw->exist = 0;
>>
>>        device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
> 
> This does not destroy any open file descriptor and we haven't
> registered any kind of hook so hidraw_destroy() will not be called
> here.
> This seems safe to me.
> We also do not check for hidraw->exist on *_open() callback so
> including this in the critical section seems fine.

That's good then. Thanks for checking it.

> 
>> -       mutex_lock(&minors_lock);
>>        hidraw_table[hidraw->minor] = NULL;
>> -       mutex_unlock(&minors_lock);
>>
>>        if (hidraw->open) {
>>                hid_hw_close(hid);
>> @@ -524,6 +523,7 @@ void hidraw_disconnect(struct hid_device *hid)
>>        } else {
>>                kfree(hidraw);
>>        }
>> +       mutex_unlock(&minors_lock);
>>  }
>>  EXPORT_SYMBOL_GPL(hidraw_disconnect);
>>
>> --
>> 1.7.2.3
> 
> Nice catch. I've tested it on linux-next tree and I can confirm the
> bug. The fix seems ok to me.

Shall I resend without RFC? I'll add Cc: <stable@kernel.org> too if
there aren't objections.

Thanks
James

> 
> Regards
> David

Re: [RFC PATCH] hidraw: protect hidraw_disconnect() better

[Jiri Kosina]

On Tue, 20 Sep 2011, James Hogan wrote:

> >> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> >> ---
> >>  drivers/hid/hidraw.c |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> >> index c79578b..a8c2b7b 100644
> >> --- a/drivers/hid/hidraw.c
> >> +++ b/drivers/hid/hidraw.c
> >> @@ -510,13 +510,12 @@ void hidraw_disconnect(struct hid_device *hid)
> >>  {
> >>        struct hidraw *hidraw = hid->hidraw;
> >>
> >> +       mutex_lock(&minors_lock);
> >>        hidraw->exist = 0;
> >>
> >>        device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
> > 
> > This does not destroy any open file descriptor and we haven't
> > registered any kind of hook so hidraw_destroy() will not be called
> > here.
> > This seems safe to me.
> > We also do not check for hidraw->exist on *_open() callback so
> > including this in the critical section seems fine.
> 
> That's good then. Thanks for checking it.
> 
> > 
> >> -       mutex_lock(&minors_lock);
> >>        hidraw_table[hidraw->minor] = NULL;
> >> -       mutex_unlock(&minors_lock);
> >>
> >>        if (hidraw->open) {
> >>                hid_hw_close(hid);
> >> @@ -524,6 +523,7 @@ void hidraw_disconnect(struct hid_device *hid)
> >>        } else {
> >>                kfree(hidraw);
> >>        }
> >> +       mutex_unlock(&minors_lock);
> >>  }
> >>  EXPORT_SYMBOL_GPL(hidraw_disconnect);
> >>
> >> --
> >> 1.7.2.3
> > 
> > Nice catch. I've tested it on linux-next tree and I can confirm the
> > bug. The fix seems ok to me.

Good, thanks. I have now applied it with your Tested-by:, please shout if 
you don't want that.

> Shall I resend without RFC? I'll add Cc: <stable@kernel.org> too if
> there aren't objections.

stable@kernel.org wouldn't work anyway due to kernel.org being down ...

-- 
Jiri Kosina
SUSE Labs