linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: James Hogan <james.hogan@imgtec.com>
To: David Herrmann <dh.herrmann@googlemail.com>
Cc: Jiri Kosina <jkosina@suse.cz>,
	linux-input@vger.kernel.org,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] hidraw: protect hidraw_disconnect() better
Date: Tue, 20 Sep 2011 09:47:05 +0100	[thread overview]
Message-ID: <4E785309.2000807@imgtec.com> (raw)
In-Reply-To: <CANq1E4QRkdd_nTORBKshBBX1tn8JxQ1U4OPaogBcrM7mOnuRhQ@mail.gmail.com>

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


  reply	other threads:[~2011-09-20  8:47 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-09-19 14:42 [RFC PATCH] hidraw: protect hidraw_disconnect() better James Hogan
2011-09-19 21:39 ` James Hogan
2011-09-20  8:36 ` David Herrmann
2011-09-20  8:47   ` James Hogan [this message]
2011-09-20 13:25     ` Jiri Kosina

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4E785309.2000807@imgtec.com \
    --to=james.hogan@imgtec.com \
    --cc=dh.herrmann@googlemail.com \
    --cc=jkosina@suse.cz \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).