linux-kernel - Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature Reports from hidraw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 28 Sep 2010 15:30:11 +0200
From:	Antonio Ospite <ospite@...denti.unina.it>
To:	Alan Ott <alan@...nal11.us>
Cc:	Jiri Kosina <jkosina@...e.cz>,
	Stefan Achatz <erazor_de@...rs.sourceforge.net>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Tejun Heo <tj@...nel.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Marcel Holtmann <marcel@...tmann.org>,
	Stephane Chatty <chatty@...c.fr>,
	Michael Poole <mdpoole@...ilus.org>,
	"David S. Miller" <davem@...emloft.net>,
	Bastien Nocera <hadess@...ess.net>,
	Eric Dumazet <eric.dumazet@...il.com>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, linux-bluetooth@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting Feature
 Reports from hidraw

On Mon, 16 Aug 2010 16:20:58 -0400
Alan Ott <alan@...nal11.us> wrote:

> Per the HID Specification, Feature reports must be sent and received on
> the Configuration endpoint (EP 0) through the Set_Report/Get_Report
> interfaces.  This patch adds two ioctls to hidraw to set and get feature
> reports to and from the device.  Modifications were made to hidraw and
> usbhid.
> 
> New hidraw ioctls:
>   HIDIOCSFEATURE - Perform a Set_Report transfer of a Feature report.
>   HIDIOCGFEATURE - Perform a Get_Report transfer of a Feature report.
> 
> Signed-off-by: Alan Ott <alan@...nal11.us>

Hi Alan, I am doing some stress testing on hidraw, if I have a loop with
HIDIOCGFEATURE on a given report and I disconnect the device while the
loop is running I get this:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]

Full log attached along with the test program, the device is a Sony PS3
Controller (sixaxis).

If my objdump analysis is right, hidraw_ioctl+0xfc should be around line
361 in hidraw.c (with your patch applied):

struct hid_device *hid = dev->hid;

It looks like 'dev' (which is hidraw_table[minor]) can be NULL
sometimes, can't it?
This is not introduced by your changes tho.

Just as a side note, the bug does not show up if the userspace program
handles return values properly and exits as soon as it gets an error
from the HID layer, see the XXX comment in test_hidraw_feature.c.

This fixes it, if it looks ok I will resend the patch rebased on
mainline code:

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 7df1310..3c040c6 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -322,6 +322,10 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,

        mutex_lock(&minors_lock);
        dev = hidraw_table[minor];
+       if (!dev) {
+               ret = -ENODEV;
+               goto out;
+       }

        switch (cmd) {
                case HIDIOCGRDESCSIZE:
@@ -412,6 +416,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,

                ret = -ENOTTY;
        }
+out:
        mutex_unlock(&minors_lock);
        return ret;
 }


this change covers also the other uses of hidraw_table[minor] in
hidraw_send_report/hidraw_get_report.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Download attachment "dmesg_hidraw_feature_bug.log" of type "application/octet-stream" (3914 bytes)

View attachment "test_hidraw_feature.c" of type "text/x-csrc" (1198 bytes)

Content of type "application/pgp-signature" skipped