linux-kernel - Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

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, 22 May 2012 10:29:05 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	Hans de Goede <hdegoede@...hat.com>
CC:	Jiri Kosina <jkosina@...e.cz>, linux-kernel@...r.kernel.org,
	USB list <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

On 05/22/2012 10:09 AM, Hans de Goede wrote:
> Hi,
> 
> On 05/22/2012 09:56 AM, Jiri Slaby wrote:
>> On 05/21/2012 09:39 PM, Hans de Goede wrote:
>>> other drivers which rely on frameworks which only call dev_set_drvdata
>>> on the interface's device if no drvdata has been set
>>
>> This looks very broken as it relies on an undocumented behavior.
> 
> I don't see how expecting intfdata to be NULL when an USB driver's
> probe function gets called is broken, esp. since most
> USB drivers will unconditionally set intfdata to something from
> their probe functions, so it seems reasonable to assume that it is
> not pointing to anything before probe gets called.

As you can see, it is not.

>> If they
>> want to do that they should:
>> * set intfdata to NULL
>> * call some hook that may set intfdata
>> * set intfdata to whatever they want if it is still NULL
>>
>> Right?
> 
> Wrong, why would they need to explictly NULL intfdata?

Because they test it against NULL later?

> it should
> be NULL when their probe gets called.

No, this is not documented anywhere as far as I can see. And many
drivers just don't do that. The same for PCI and likely other buses
(like HID).

> I cannot believe we are
> even having this discussion, are you really trying to argue
> that leaving intfdata as a dangling pointer, rather then setting
> it to NULL (*) is better???

No, the patch looks correct. But you are expecting something which is
not still guaranteed.

>> What are those frameworks?
> v4l2-device.c does this, the call sequence is:
> 
> -some driver's probe method gets called
> -that driver can either call dev_set_drvdata itself, if it has its
>  own uses for it, or leave it as is (which should be NULL at this point)

(No, it should not. Change all the drivers or the USB and PCI cores,
document that and then you can expect it.)

> -v4l2_device_register gets called on a v4l2_device struct, with a
>  device argument, if that device does not have any drvdata set, it will
>  set drvdata to point to the v4l2_device struct.

As it stands, v4l now actually depends on its drivers to set intfdata
unconditionally. Either to NULL or some valid pointer...

regards,
-- 
js
suse labs


--
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/