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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/