rtl8xxxu 4.4.5(from f23): I get a panic adding a new device to the driver

Commit Message

Jes Sorensen March 17, 2016, 6:02 p.m. UTC

Jes Sorensen <[e-mail]> writes:
> Xose Vazquez Perez <[e-mail]> writes:
>> Hi,
>>
>> If I do:
>> # echo "0bda 8176" > /sys/bus/usb/drivers/rtl8xxxu/new_id
>
> Hi Xose,
>
> Yes please don't do that. The rtl8xxxu driver relies on the .driver_info
> field in struct use_device_id to carry information for the different
> types of devices. If you hot add a device like above, the driver will
> fail because that field now contains a NULL pointer.
>
> I should probably add a check for it in the probe function, but it will
> simply be there to spit out a warning that it doesn't work to hot add a
> device like this.
>
> If you build it with CONFIG_RTL8XXXU_UNTESTED the 0bda:8176 should be
> included in the device list.
>
> Cheers,
> Jes

Hi Xose,

I added the following patch to my tree to avoid this.

Cheers,
Jes

commit 9202f4947aac1d60084ee79c9b5294eb42ba59dc
Author: Jes Sorensen <[e-mail]>
Date:   Thu Mar 17 13:53:48 2016 -0400

    rtl8xxxu: Fix OOPS if user tries to add device via /sys
    
    This driver relies on driver_info in struct usb_device_id, hence
    adding a device via  /sys/bus/usb/drivers/rtl8xxxu/new_id would result
    in a NULL pointer dereference.
    
    Instead print a message and return -ENODEV
    
    Reported-by: Xose Vazquez Perez <[e-mail]>
    Signed-off-by: Jes Sorensen <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

poma March 17, 2016, 8:01 p.m. UTC | #1

On 17.03.2016 19:02, Jes Sorensen wrote:
> Jes Sorensen <[e-mail]> writes:
>> Xose Vazquez Perez <[e-mail]> writes:
>>> Hi,
>>>
>>> If I do:
>>> # echo "0bda 8176" > /sys/bus/usb/drivers/rtl8xxxu/new_id
>>
>> Hi Xose,
>>
>> Yes please don't do that. The rtl8xxxu driver relies on the .driver_info
>> field in struct use_device_id to carry information for the different
>> types of devices. If you hot add a device like above, the driver will
>> fail because that field now contains a NULL pointer.
>>
>> I should probably add a check for it in the probe function, but it will
>> simply be there to spit out a warning that it doesn't work to hot add a
>> device like this.
>>
>> If you build it with CONFIG_RTL8XXXU_UNTESTED the 0bda:8176 should be
>> included in the device list.
>>
>> Cheers,
>> Jes
> 
> Hi Xose,
> 
> I added the following patch to my tree to avoid this.
> 
> Cheers,
> Jes
> 
> commit 9202f4947aac1d60084ee79c9b5294eb42ba59dc
> Author: Jes Sorensen <[e-mail]>
> Date:   Thu Mar 17 13:53:48 2016 -0400
> 
>     rtl8xxxu: Fix OOPS if user tries to add device via /sys
>     
>     This driver relies on driver_info in struct usb_device_id, hence
>     adding a device via  /sys/bus/usb/drivers/rtl8xxxu/new_id would result
>     in a NULL pointer dereference.
>     
>     Instead print a message and return -ENODEV
>     
>     Reported-by: Xose Vazquez Perez <[e-mail]>
>     Signed-off-by: Jes Sorensen <[e-mail]>
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> index 8d893f4..55fc00e 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> @@ -9671,6 +9671,15 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>  
>  	udev = usb_get_dev(interface_to_usbdev(interface));
>  
> +	if (!id->driver_info) {
> +		dev_warn(&udev->dev,
> +			 "rtl8xxxu relies on driver_info in struct usb_device_id.\n");
> +		dev_warn(&udev->dev,
> +			 "Adding a device via /sys/bus/usb/drivers/rtl8xxxu/new_id is not supported!\n");
> +		ret = -ENODEV;
> +		goto exit;
> +	}
> +
>  	switch (id->idVendor) {
>  	case USB_VENDOR_ID_REALTEK:
>  		switch(id->idProduct) {
>


Dynamic adding and removing a new device IDs to a USB device drivers
via /sys/bus/usb/drivers/.../[new_id|remove_id]
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-usb

is not supported only with rtl8xxxu?


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jes Sorensen March 17, 2016, 8:11 p.m. UTC | #2

poma <[e-mail]> writes:
> On 17.03.2016 19:02, Jes Sorensen wrote:
>> Jes Sorensen <[e-mail]> writes:
>>> Xose Vazquez Perez <[e-mail]> writes:
>>>> Hi,
>>>>
>>>> If I do:
>>>> # echo "0bda 8176" > /sys/bus/usb/drivers/rtl8xxxu/new_id
>>>
>>> Hi Xose,
>>>
>>> Yes please don't do that. The rtl8xxxu driver relies on the .driver_info
>>> field in struct use_device_id to carry information for the different
>>> types of devices. If you hot add a device like above, the driver will
>>> fail because that field now contains a NULL pointer.
>>>
>>> I should probably add a check for it in the probe function, but it will
>>> simply be there to spit out a warning that it doesn't work to hot add a
>>> device like this.
>>>
>>> If you build it with CONFIG_RTL8XXXU_UNTESTED the 0bda:8176 should be
>>> included in the device list.
>>>
>>> Cheers,
>>> Jes
>> 
>> Hi Xose,
>> 
>> I added the following patch to my tree to avoid this.
>> 
>> Cheers,
>> Jes
>> 
>> commit 9202f4947aac1d60084ee79c9b5294eb42ba59dc
>> Author: Jes Sorensen <[e-mail]>
>> Date:   Thu Mar 17 13:53:48 2016 -0400
>> 
>>     rtl8xxxu: Fix OOPS if user tries to add device via /sys
>>     
>>     This driver relies on driver_info in struct usb_device_id, hence
>>     adding a device via  /sys/bus/usb/drivers/rtl8xxxu/new_id would result
>>     in a NULL pointer dereference.
>>     
>>     Instead print a message and return -ENODEV
>>     
>>     Reported-by: Xose Vazquez Perez <[e-mail]>
>>     Signed-off-by: Jes Sorensen <[e-mail]>
>> 
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
>> index 8d893f4..55fc00e 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
>> @@ -9671,6 +9671,15 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>>  
>>  	udev = usb_get_dev(interface_to_usbdev(interface));
>>  
>> +	if (!id->driver_info) {
>> +		dev_warn(&udev->dev,
>> +			 "rtl8xxxu relies on driver_info in struct usb_device_id.\n");
>> +		dev_warn(&udev->dev,
>> +			 "Adding a device via /sys/bus/usb/drivers/rtl8xxxu/new_id is not supported!\n");
>> +		ret = -ENODEV;
>> +		goto exit;
>> +	}
>> +
>>  	switch (id->idVendor) {
>>  	case USB_VENDOR_ID_REALTEK:
>>  		switch(id->idProduct) {
>
> Dynamic adding and removing a new device IDs to a USB device drivers
> via /sys/bus/usb/drivers/.../[new_id|remove_id]
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-usb
>
> is not supported only with rtl8xxxu?

Dynamically adding one will not work. The driver relies on driver_info
in struct usb_device_id() to carry the struct fileops associated with
the device type and you cannot specify this via the USB add operation.

Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Greg KH March 17, 2016, 8:42 p.m. UTC | #3

On Thu, Mar 17, 2016 at 09:01:25PM +0100, poma wrote:
> On 17.03.2016 19:02, Jes Sorensen wrote:
> > Jes Sorensen <[e-mail]> writes:
> >> Xose Vazquez Perez <[e-mail]> writes:
> >>> Hi,
> >>>
> >>> If I do:
> >>> # echo "0bda 8176" > /sys/bus/usb/drivers/rtl8xxxu/new_id
> >>
> >> Hi Xose,
> >>
> >> Yes please don't do that. The rtl8xxxu driver relies on the .driver_info
> >> field in struct use_device_id to carry information for the different
> >> types of devices. If you hot add a device like above, the driver will
> >> fail because that field now contains a NULL pointer.
> >>
> >> I should probably add a check for it in the probe function, but it will
> >> simply be there to spit out a warning that it doesn't work to hot add a
> >> device like this.
> >>
> >> If you build it with CONFIG_RTL8XXXU_UNTESTED the 0bda:8176 should be
> >> included in the device list.
> >>
> >> Cheers,
> >> Jes
> > 
> > Hi Xose,
> > 
> > I added the following patch to my tree to avoid this.
> > 
> > Cheers,
> > Jes
> > 
> > commit 9202f4947aac1d60084ee79c9b5294eb42ba59dc
> > Author: Jes Sorensen <[e-mail]>
> > Date:   Thu Mar 17 13:53:48 2016 -0400
> > 
> >     rtl8xxxu: Fix OOPS if user tries to add device via /sys
> >     
> >     This driver relies on driver_info in struct usb_device_id, hence
> >     adding a device via  /sys/bus/usb/drivers/rtl8xxxu/new_id would result
> >     in a NULL pointer dereference.
> >     
> >     Instead print a message and return -ENODEV
> >     
> >     Reported-by: Xose Vazquez Perez <[e-mail]>
> >     Signed-off-by: Jes Sorensen <[e-mail]>
> > 
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> > index 8d893f4..55fc00e 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> > @@ -9671,6 +9671,15 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
> >  
> >  	udev = usb_get_dev(interface_to_usbdev(interface));
> >  
> > +	if (!id->driver_info) {
> > +		dev_warn(&udev->dev,
> > +			 "rtl8xxxu relies on driver_info in struct usb_device_id.\n");
> > +		dev_warn(&udev->dev,
> > +			 "Adding a device via /sys/bus/usb/drivers/rtl8xxxu/new_id is not supported!\n");

We do have a flag in the USB driver structure to prevent this from
happening, "no_dynamic_id", please just set that and then this should
not happen.

thanks,

gre k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Stern March 17, 2016, 8:50 p.m. UTC | #4

On Thu, 17 Mar 2016, poma wrote:

> On 17.03.2016 19:02, Jes Sorensen wrote:
> > Jes Sorensen <[e-mail]> writes:
> >> Xose Vazquez Perez <[e-mail]> writes:
> >>> Hi,
> >>>
> >>> If I do:
> >>> # echo "0bda 8176" > /sys/bus/usb/drivers/rtl8xxxu/new_id
> >>
> >> Hi Xose,
> >>
> >> Yes please don't do that. The rtl8xxxu driver relies on the .driver_info
> >> field in struct use_device_id to carry information for the different
> >> types of devices. If you hot add a device like above, the driver will
> >> fail because that field now contains a NULL pointer.
> >>
> >> I should probably add a check for it in the probe function, but it will
> >> simply be there to spit out a warning that it doesn't work to hot add a
> >> device like this.
> >>
> >> If you build it with CONFIG_RTL8XXXU_UNTESTED the 0bda:8176 should be
> >> included in the device list.
> >>
> >> Cheers,
> >> Jes
> > 
> > Hi Xose,
> > 
> > I added the following patch to my tree to avoid this.
> > 
> > Cheers,
> > Jes
> > 
> > commit 9202f4947aac1d60084ee79c9b5294eb42ba59dc
> > Author: Jes Sorensen <[e-mail]>
> > Date:   Thu Mar 17 13:53:48 2016 -0400
> > 
> >     rtl8xxxu: Fix OOPS if user tries to add device via /sys
> >     
> >     This driver relies on driver_info in struct usb_device_id, hence
> >     adding a device via  /sys/bus/usb/drivers/rtl8xxxu/new_id would result
> >     in a NULL pointer dereference.
> >     
> >     Instead print a message and return -ENODEV
> >     
> >     Reported-by: Xose Vazquez Perez <[e-mail]>
> >     Signed-off-by: Jes Sorensen <[e-mail]>
> > 
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> > index 8d893f4..55fc00e 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> > @@ -9671,6 +9671,15 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
> >  
> >  	udev = usb_get_dev(interface_to_usbdev(interface));
> >  
> > +	if (!id->driver_info) {
> > +		dev_warn(&udev->dev,
> > +			 "rtl8xxxu relies on driver_info in struct usb_device_id.\n");

You should leave out this line.  It won't mean anything to the general
user.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jes Sorensen March 17, 2016, 9:47 p.m. UTC | #5

Greg KH <[e-mail]> writes:
> On Thu, Mar 17, 2016 at 09:01:25PM +0100, poma wrote:
>> On 17.03.2016 19:02, Jes Sorensen wrote:
>> > Jes Sorensen <[e-mail]> writes:
>> >> Xose Vazquez Perez <[e-mail]> writes:
>> >>> Hi,
>> >>>
>> >>> If I do:
>> >>> # echo "0bda 8176" > /sys/bus/usb/drivers/rtl8xxxu/new_id
>> >>
>> >> Hi Xose,
>> >>
>> >> Yes please don't do that. The rtl8xxxu driver relies on the .driver_info
>> >> field in struct use_device_id to carry information for the different
>> >> types of devices. If you hot add a device like above, the driver will
>> >> fail because that field now contains a NULL pointer.
>> >>
>> >> I should probably add a check for it in the probe function, but it will
>> >> simply be there to spit out a warning that it doesn't work to hot add a
>> >> device like this.
>> >>
>> >> If you build it with CONFIG_RTL8XXXU_UNTESTED the 0bda:8176 should be
>> >> included in the device list.
>> >>
>> >> Cheers,
>> >> Jes
>> > 
>> > Hi Xose,
>> > 
>> > I added the following patch to my tree to avoid this.
>> > 
>> > Cheers,
>> > Jes
>> > 
>> > commit 9202f4947aac1d60084ee79c9b5294eb42ba59dc
>> > Author: Jes Sorensen <[e-mail]>
>> > Date:   Thu Mar 17 13:53:48 2016 -0400
>> > 
>> >     rtl8xxxu: Fix OOPS if user tries to add device via /sys
>> >     
>> >     This driver relies on driver_info in struct usb_device_id, hence
>> >     adding a device via  /sys/bus/usb/drivers/rtl8xxxu/new_id would result
>> >     in a NULL pointer dereference.
>> >     
>> >     Instead print a message and return -ENODEV
>> >     
>> >     Reported-by: Xose Vazquez Perez <[e-mail]>
>> >     Signed-off-by: Jes Sorensen <[e-mail]>
>> > 
>> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
>> > b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
>> > index 8d893f4..55fc00e 100644
>> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
>> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
>> > @@ -9671,6 +9671,15 @@ static int rtl8xxxu_probe(struct
>> > usb_interface *interface,
>> >  
>> >  	udev = usb_get_dev(interface_to_usbdev(interface));
>> >  
>> > +	if (!id->driver_info) {
>> > +		dev_warn(&udev->dev,
>> > + "rtl8xxxu relies on driver_info in struct usb_device_id.\n");
>> > +		dev_warn(&udev->dev,
>> > + "Adding a device via /sys/bus/usb/drivers/rtl8xxxu/new_id is not
>> > supported!\n");
>
> We do have a flag in the USB driver structure to prevent this from
> happening, "no_dynamic_id", please just set that and then this should
> not happen.

I wasn't aware of this flag - I'll update the patch to do that.

Thanks,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
index 8d893f4..55fc00e 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
@@ -9671,6 +9671,15 @@  static int rtl8xxxu_probe(struct usb_interface *interface,
 
 	udev = usb_get_dev(interface_to_usbdev(interface));
 
+	if (!id->driver_info) {
+		dev_warn(&udev->dev,
+			 "rtl8xxxu relies on driver_info in struct usb_device_id.\n");
+		dev_warn(&udev->dev,
+			 "Adding a device via /sys/bus/usb/drivers/rtl8xxxu/new_id is not supported!\n");
+		ret = -ENODEV;
+		goto exit;
+	}
+
 	switch (id->idVendor) {
 	case USB_VENDOR_ID_REALTEK:
 		switch(id->idProduct) {