hidraw: protect hidraw_disconnect() better

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 <[email protected]>

---
 drivers/hid/hidraw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/hid/hidraw.c +2 -2

index c79578b..a8c2b7b 100644
@@ -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);
 

Signature · 7 lines

1.7.2.3


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

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 <[email protected]>

---
 drivers/hid/hidraw.c |    4 ++--

 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/hid/hidraw.c +0 -0

index c79578b..a8c2b7b 100644

drivers/hid/hidraw.c +1 -3

-       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

Hi James

Quoted 41 lines · 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 <[email protected]>

---
 drivers/hid/hidraw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/hid/hidraw.c +1 -0

index c79578b..a8c2b7b 100644
@@ -510,13 +510,12 @@ void hidraw_disconnect(struct hid_device *hid)
 {
       struct hidraw *hidraw = hid->hidraw;
 
+       mutex_lock(&minors_lock);
       hidraw->exist = 0;
 

drivers/hid/hidraw.c void hidraw_disconnect(struct hid_device *hid) · line 516

Quoted 1 line

drivers/hid/hidraw.c +0 -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.

Quoted 15 lines

drivers/hid/hidraw.c +1 -2

-       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);
 

Quoted 2 lines

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

Quoted 16 lines

Quoted 16 lines

Signed-off-by James Hogan <[email protected]>

---
 drivers/hid/hidraw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/hid/hidraw.c +1 -0

index c79578b..a8c2b7b 100644
@@ -510,13 +510,12 @@ void hidraw_disconnect(struct hid_device *hid)
 {
       struct hidraw *hidraw = hid->hidraw;
 
+       mutex_lock(&minors_lock);
       hidraw->exist = 0;
 

Quoted 8 lines

drivers/hid/hidraw.c void hidraw_disconnect(struct hid_device *hid) · line 516

Quoted 1 line

drivers/hid/hidraw.c +0 -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.

Quoted 20 lines

Quoted 16 lines

drivers/hid/hidraw.c +1 -3

-       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);
 
--

Quoted 1 line

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: <[email protected]> too if there aren't objections.