linux-kernel - Re: [PATCH v4] [RFC] NEXIO (or iNexio) support for usbtouchscreen

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: <200911201943.46696.oliver@neukum.org>
Date:	Fri, 20 Nov 2009 19:43:46 +0100
From:	Oliver Neukum <oliver@...kum.org>
To:	Ondrej Zary <linux@...nbow-software.org>
Cc:	linux-usb@...r.kernel.org, daniel.ritz@....ch,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] [RFC] NEXIO (or iNexio) support for usbtouchscreen

Am Freitag, 20. November 2009 10:21:43 schrieb Ondrej Zary:
> On Thursday 19 November 2009, Oliver Neukum wrote:
> > > +struct nexio_priv {
> > > +	struct urb *ack;
> > > +	char ack_buf[2];
> > > +};
> >
> > No. Every buffer needs to have an exclusive cache line for DMA
> > to work on the incoherent archotectures. Therefore you must allocate
> > each buffer with its own kmalloc.
> 
> OK, thanks for your patience.

No problem. I should have explained better.

> > > +	/* read replies */
> > > +	for (i = 0; i < 3; i++) {
> > > +		memset(buf, 0, NEXIO_BUFSIZE);
> > > +		ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, NEXIO_INPUT_EP),
> > > +				   buf, NEXIO_BUFSIZE, &actual_len,
> > > +				   NEXIO_TIMEOUT);
> > > +		if (ret < 0 || actual_len < 1 || buf[1] != actual_len)
> > > +			continue;
> > > +		switch (buf[0]) {
> > > +			case 0x83:	/* firmware version */
> > > +				firmware_ver = kstrdup(&buf[2], GFP_KERNEL);

On second thought this is not nice. If a device is broken enough to report
a name or a firmware version twice, you produce a memory leak.
Do you know very buggy devices to exist?

> > > +				break;
> > > +			case 0x84:	/* device name */
> > > +				device_name = kstrdup(&buf[2], GFP_KERNEL);
> > > +				break;
> > > +		}
> > > +	}
> > > +	printk(KERN_INFO "Nexio device: %s, firmware version: %s\n",
> > > +	       device_name, firmware_ver);
> >
> > How do you know device_name and firmware_ver are not NULL?
> 
> printk works fine with NULL, it prints <NULL>. Is it necessary to add more
> code only to make the output nice?

No, for niceness it is not necessary. The question is whether you want
to treat this as an error or print a warning. That is a matter of taste.
 

 
> Looks like a bug in the original usbtouchscreen code. There's no locking.
> Will a spinlock in usbtouch_open() and usbtouch_disconnect() fix it? Or
> do you see more problems here?

You must not call usb_kill_urb() with a spinlock held.
I'll lokk at the usbtouchscreen code.
The new version looks good to me.

	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/