Bug 239973

Summary: Kernel Panic: device_get_ivars(9) returns NULL which leads to Null pointer dereference for multiple drivers
Product: Base System Reporter: Neeraj <neerajpal09>
Component: kernAssignee: freebsd-bugs (Nobody) <bugs>
Status: New ---    
Severity: Affects Many People CC: grahamperrin, hselasky, neerajpal09
Priority: --- Keywords: crash, patch
Version: CURRENT   
Hardware: Any   
OS: Any   
URL: https://marc.info/?l=freebsd-arch&m=156572523104632&w=2
Attachments:
Description Flags
kernel panic log
none
proposed patch diff for 5 drivers mentioned in the bug none

Description Neeraj 2019-08-19 19:50:55 UTC
Created attachment 206706 [details]
kernel panic log

Kernel Panic is observed for NULL pointer dereference in FreeBSD
kernel driver code due to which kernel gets in panic then it has to reboot.

Actually, this vulnerability resides in lots of kernel drivers like
"uhub0", "ubt0", "umass0", "run0", "uhid0" etc., mostly usb devices.

Tested and observed the panic for following kernel drivers:

  - usb,
  - umass (storage),
  - ubt(bluetooth),
  - run0(wifi),
  - uhid

Devices which are using the structure
"usb_attach_arg" with device_get_ivars(9) as mentioned
below:
"struct usb_attach_arg *uaa = device_get_ivars(dev)"
are prone to NULL pointer dereference bug as there is no check
for the same and the api device_get_ivars(9) is returning NULL.

device_get_ivars(9) from the file
"/usr/src/sys/kern/subr_bus.c" returns a NULL pointer, which get assigned
to *uaa structure object (function "uhub_probe" from file
"/usr/src/sys/dev/usb/usb_bus.c"),
then, after that there is a if-else condition which is checking the
usb_mode from that structure and there panic occurs due to dereferencing
the NULL pointer
Same valid for other kernel drivers.

There are still lots of drivers which are lacking this NULL pointer
dereference check, apart from what mentioned here.

[steps to reproduce]
* devctl disable uhub0
* devctl enable uhub0 <= BOOM - panic appears

Panic occurs here, after enabling the already disabled device (but only
with usb related drivers)

[Privilege]
Root privilege is required.

[Reproducibility]
Reproducibility is 100%

[Log]
Attached file "panic.log"

[Patch]
Please find the attached patch file "patch.diff" for the file "usb_hub.c", "ng_ubt.c",
"if_run.c", "umass.c" and "uhid.c"

After applying the patch, it first returns the "ENXIO" as mentioned in the
patch code then later invocation returns "EBUSY" as device is enabled,
which can be verified by disabling it again.
Comment 1 Neeraj 2019-08-19 19:52:47 UTC
Created attachment 206707 [details]
proposed patch diff for 5 drivers mentioned in the bug
Comment 2 Hans Petter Selasky freebsd_committer freebsd_triage 2019-09-05 13:22:11 UTC
This patch is not correct for USB. Please blacklist devctl from touching USB device nodes or anything that parents to a usbusX.
Comment 3 Neeraj 2019-09-15 14:12:49 UTC
(In reply to Hans Petter Selasky from comment #2)

yeah you right. Actually, as per the discussion going in the mailing list, other developers said that the patch is correct *if and only if want to preserve the ivars information* and preserving the info would be a long term solution.

But, as you said that there might be a race, that devctl is 
attaching/detaching drivers while USB is doing the same, it might go into the panic due to that.

So,  you are correct on the point that USB drivers are not supposed to be managed outside the USB enumeration thread. Using devctl on USB driver is not supported. Only usbconfig is allowed to attach/detach USB devices.

And, I also think that it is a better solution to blacklist devctl from touching USB device nodes or anything that parents to a usbusX, instead of patching all the devices. As, we already have usbconfig to do the same.