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:
pgp_yxD38EaT3.pgp
Description: PGP signature