Re: [PATCH v4 1/2] HID: Add Support for Setting and Getting FeatureReports from hidraw

[Antonio Ospite]

On Mon, 16 Aug 2010 16:20:58 -0400
Alan Ott <alan@xxxxxxxxxxx> 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@xxxxxxxxxxx>

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?

Attachment: dmesg_hidraw_feature_bug.log
Description: Binary data

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>

/*
#include <linux/hidraw.h>
*/
#include "/home/ao2/Proj/linux/linux-2.6/include/linux/hidraw.h"

void dump_hex_string(unsigned char *buf, unsigned int len)
{
	unsigned int i;

	for (i = 0; i < len; i++)
		printf("%02x%c", buf[i], i < len - 1 ? ' ' : 0);
}

void dump_feature_report(int fd, uint8_t report_number, unsigned int len)
{
	unsigned char *buf;
	int ret;

	buf = calloc(len, sizeof(*buf));

	buf[0] = report_number;

	ret = ioctl(fd, HIDIOCGFEATURE(len), buf);
	if (ret < 0) {
		fprintf(stderr, "report: 0x%02x ret: %d\n", report_number, ret);
		/* XXX: if I put exit(1) here the bug is masked */
		return;
	}

	dump_hex_string(buf, len);
	printf("\r");
	fflush(stdout);

	free(buf);
}

int main(int argc, char *argv[])
{
	int fd = -1;

	if (argc != 2) {
		fprintf(stderr, "usage: %s </dev/hidrawX>\n", argv[0]);
		exit(1);
	}

	fd = open(argv[1], O_RDWR);
	if (fd < 0) {
		perror("hidraw open");
		exit(1);
	}

	while (1)
		dump_feature_report(fd, 0x01, 45);
	printf("\n");

	close(fd);
	exit(0);
}

Attachment: pgp00000.pgp
Description: PGP signature