Skip to contentSkip to conversation

udlfb: fix some inconsistent NULL checking

PATCH In review Dan Carpenter 2 participants 2 messages Oct 10, 2018 – Dec 20, 2018

Subsystems (3)
Kernel janitors Framebuffer layer Displaylink USB 2.0 framebuffer driver (udlfb)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 udlfb: fix some inconsistent NULL checking

In the current kernel, then kzalloc() can't fail for small allocations, but if it did fail then we would have a NULL dereference in the error handling. Also in dlfb_usb_disconnect() if "info" were NULL then it would cause an Oops inside the unregister_framebuffer() function but it can't be NULL so let's remove that check.

Signed-off-by Dan Carpenter <[email protected]>
drivers/video/fbdev/udlfb.c +6 -8
index 070026a7e55a..1d034dddc556 100644
@@ -1598,7 +1598,7 @@ static int dlfb_usb_probe(struct usb_interface *intf,
 	dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
 	if (!dlfb) {
 		dev_err(&intf->dev, "%s: failed to allocate dlfb\n", __func__);
-		goto error;
+		return -ENOMEM;
 	}
 
 	INIT_LIST_HEAD(&dlfb->deferred_free);

@@ -1703,7 +1703,7 @@ static int dlfb_usb_probe(struct usb_interface *intf,
 error:
 	if (dlfb->info) {
 		dlfb_ops_destroy(dlfb->info);
-	} else if (dlfb) {
+	} else {
 		usb_put_dev(dlfb->udev);
 		kfree(dlfb);
 	}

@@ -1730,12 +1730,10 @@ static void dlfb_usb_disconnect(struct usb_interface *intf)
 	/* this function will wait for all in-flight urbs to complete */
 	dlfb_free_urb_list(dlfb);
 
-	if (info) {
-		/* remove udlfb's sysfs interfaces */
-		for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++)
-			device_remove_file(info->dev, &fb_device_attrs[i]);
-		device_remove_bin_file(info->dev, &edid_attr);
-	}
+	/* remove udlfb's sysfs interfaces */
+	for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++)
+		device_remove_file(info->dev, &fb_device_attrs[i]);
+	device_remove_bin_file(info->dev, &edid_attr);
 
 	unregister_framebuffer(info);
 }
#1 lore raw

[ added Mikulas, Alexey, Colin & Wen to Cc: ]

Quoted 7 lines · Dan Carpenter

In the current kernel, then kzalloc() can't fail for small allocations, but if it did fail then we would have a NULL dereference in the error handling. Also in dlfb_usb_disconnect() if "info" were NULL then it would cause an Oops inside the unregister_framebuffer() function but it can't be NULL so let's remove that check.

Signed-off-by Dan Carpenter <[email protected]>

Patch queued for 4.21, thanks (also sorry for the delay).

[ Mikulas, Alexey, Colin & Wen: please verify that there are no known
issues with udlfb left with this patch applied (thanks!). ]

Quoted 29 lines
drivers/video/fbdev/udlfb.c +2 -5
index 070026a7e55a..1d034dddc556 100644
@@ -1598,7 +1598,7 @@ static int dlfb_usb_probe(struct usb_interface *intf,
 	dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
 	if (!dlfb) {
 		dev_err(&intf->dev, "%s: failed to allocate dlfb\n", __func__);
-		goto error;
+		return -ENOMEM;
 	}
 
 	INIT_LIST_HEAD(&dlfb->deferred_free);

@@ -1703,7 +1703,7 @@ static int dlfb_usb_probe(struct usb_interface *intf,
 error:
 	if (dlfb->info) {
 		dlfb_ops_destroy(dlfb->info);
-	} else if (dlfb) {
+	} else {
 		usb_put_dev(dlfb->udev);
 		kfree(dlfb);
 	}

@@ -1730,12 +1730,10 @@ static void dlfb_usb_disconnect(struct usb_interface *intf)
 	/* this function will wait for all in-flight urbs to complete */
 	dlfb_free_urb_list(dlfb);
 
-	if (info) {
-		/* remove udlfb's sysfs interfaces */
-		for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++)

drivers/video/fbdev/udlfb.c static void dlfb_usb_disconnect(struct usb_interface *intf) · line 1739

Quoted 10 lines
drivers/video/fbdev/udlfb.c +4 -3
-			device_remove_file(info->dev, &fb_device_attrs[i]);
-		device_remove_bin_file(info->dev, &edid_attr);
-	}
+	/* remove udlfb's sysfs interfaces */
+	for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++)
+		device_remove_file(info->dev, &fb_device_attrs[i]);
+	device_remove_bin_file(info->dev, &edid_attr);
 
 	unregister_framebuffer(info);
 }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics