linux-kernel - Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

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]

Message-Id: <200902160931.34771.oliver@neukum.org>
Date:	Mon, 16 Feb 2009 09:31:33 +0100
From:	Oliver Neukum <oliver@...kum.org>
To:	Mike Murphy <mamurph@...clemson.edu>, linux-usb@...r.kernel.org,
	linux-input@...r.kernel.org
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

Am Sunday 15 February 2009 05:08:46 schrieb Mike Murphy:
> Greetings,
> 
> The attached patch is a result of a few days of hacking to get better
> Linux support for Xbox 360 wireless controllers. Major functional
> improvements include support for the LEDs on the wireless controllers
> (which are auto-set to the controller number as they are on the
> official console), added support for rumble effects on wireless
> controllers, added support for dead zones on all supported
> controllers, and added support for a square axis mapping for the left
> stick on all supported controllers. A large amount of the bulk in this
> patch is from the addition of a sysfs interface to retrieve
> information and adjust the dead zone/square axis settings.
[..]
> Patch generated against 2.6.28.2 (no changes in relevant in-kernel
> driver from 2.6.28.2 to 2.6.28.5, so should apply against latest
> stable).
> 
> Comments are appreciated. I am _NOT_ subscribed to the LKML, so please
> CC me directly.

1. You need to check the returns of sscanf
2. This is very ugly:

+/* read-only attrs */
+static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
+	char *buf)
+{
+	int value;
+	if (!strcmp(attr->attr.name, "controller_number"))
+		value = xd->controller_number;
+	else if (!strcmp(attr->attr.name, "pad_present"))
+		value = xd->pad_present;
+	else if (!strcmp(attr->attr.name, "controller_type"))
+		value = xd->controller_type;
+	else
+		value = 0;
+	return sprintf(buf, "%d\n", value);
+}

3. Possible memory leak in error case:

+static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
+	struct xpad_data *data = NULL;
+	int check;
+	
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return NULL;
+	
+	check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
+	if (check) {
+		kobject_put(&data->kobj);
+		return NULL;
+	}

4. Why the cpup variety?

+	coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));

5. What happens if this work is already scheduled?

 	if (data[0] & 0x08) {
+		padnum = xpad->controller_data->controller_number;
 		if (data[1] & 0x80) {
-			xpad->pad_present = 1;
-			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
-		} else
-			xpad->pad_present = 0;
+			printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
+			xpad->controller_data->pad_present = 1;
+			
+			INIT_WORK(&xpad->work, &xpad_work_controller);
+			schedule_work(&xpad->work);

6. No GFP_ATOMIC. If you can take a mutex you can sleep.
+		usb_submit_urb(xpad->irq_out, GFP_ATOMIC);

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/