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]

Message-ID: <4FBB5D4C.9090804@redhat.com>
Date:	Tue, 22 May 2012 11:33:00 +0200
From:	Hans de Goede <hdegoede@...hat.com>
To:	Jiri Slaby <jslaby@...e.cz>
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

Hi,

On 05/22/2012 10:29 AM, Jiri Slaby wrote:
> 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.

Notice I used the word "reasoanble" I still believe it is reasonable
to expect intfdata to be NULL. intfdata is just an alias for dev_drvdata,
and if no driver is bound to a device what should its drvdata be?

So we have:
1) A device which does not have a driver bound
2) A potential drivers probe method being called (which will only
    happen if 1. is true)
3) That probe method expecting drvdata to be NULL since no driver is
    bound

Surprise surprise (not). If no driver is bound what else can drvdata
be but NULL, anything else would be a reminisce of a previous driver,
and thus very likely a dangling pointer.

>> 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).

Well on driver unbind the USB core explictly sets intfdata to NULL,
which to me clearly signals intent that intfdata should be NULL when
no driver is bound.

The usb code does not do clear intfdata on probe fail, I don't know why,
likely because it expects a failed probe to not set it in the first
place! But it probably is a very good idea to make the USB core set
intfdata to NULL after a failed probe to ensure that it is NULL
when no driver is bound, independent of driver behavior.

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

Well then lets work towards making it guaranteed, since I still believe
the following holds true:
1) drvdata is for a driver to store a pointer to driver specific data
2) If no driver is bound, there is no driver specific data associated with
    the device
3) Thus logically drvdata should be NULL if no driver is bound.

I'll do a patch for the USB-core to ensure that intfdata gets set to NULL
on probe failure.

Regards,

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