Re: [PATCH] fbdev: smscufx: fix error handling code in ufx_usb_probe
On 11/11/22 06:49, Dongliang Mu wrote:
The current error handling code in ufx_usb_probe have many unmatching
issues, e.g., missing ufx_free_usb_list, destroy_modedb label should
only include framebuffer_release, fb_dealloc_cmap only matches
fb_alloc_cmap.
My local syzkaller reports a memory leak bug:
memory leak in ufx_usb_probe
BUG: memory leak
unreferenced object 0x88802f879580 (size 128):
comm "kworker/0:7", pid 17416, jiffies 4295067474 (age 46.710s)
hex dump (first 32 bytes):
80 21 7c 2e 80 88 ff ff 18 d0 d0 0c 80 88 ff ff .!|.
00 d0 d0 0c 80 88 ff ff e0 ff ff ff 0f 00 00 00
backtrace:
[] kmalloc_trace+0x20/0x90 mm/slab_common.c:1045
[] kmalloc include/linux/slab.h:553 [inline]
[] kzalloc include/linux/slab.h:689 [inline]
[] ufx_alloc_urb_list drivers/video/fbdev/smscufx.c:1873
[inline]
[] ufx_usb_probe+0x11c/0x15a0
drivers/video/fbdev/smscufx.c:1655
[] usb_probe_interface+0x177/0x370
drivers/usb/core/driver.c:396
[] call_driver_probe drivers/base/dd.c:560 [inline]
[] really_probe+0x12d/0x390 drivers/base/dd.c:639
[] __driver_probe_device+0xbf/0x140 drivers/base/dd.c:778
[] driver_probe_device+0x2a/0x120 drivers/base/dd.c:808
[] __device_attach_driver+0xf7/0x150
drivers/base/dd.c:936
[] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
[] __device_attach+0x105/0x2d0 drivers/base/dd.c:1008
[] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
[] device_add+0x642/0xdc0 drivers/base/core.c:3517
[] usb_set_configuration+0x8ef/0xb80
drivers/usb/core/message.c:2170
[] usb_generic_driver_probe+0x8c/0xc0
drivers/usb/core/generic.c:238
[] usb_probe_device+0x5c/0x140
drivers/usb/core/driver.c:293
[] call_driver_probe drivers/base/dd.c:560 [inline]
[] really_probe+0x12d/0x390 drivers/base/dd.c:639
[] __driver_probe_device+0xbf/0x140 drivers/base/dd.c:778
Fix this bug by rewriting the error handling code in ufx_usb_probe.
Reported-by: syzkaller
Tested-by: Dongliang Mu
Signed-off-by: Dongliang Mu
applied.
Thanks!
Helge
---
drivers/video/fbdev/smscufx.c | 46 +++
1 file changed, 31 insertions(+), 15 deletions(-)
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 9343b7a4ac89..2ad6e98ce10d 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -1622,7 +1622,7 @@ static int ufx_usb_probe(struct usb_interface *interface,
struct usb_device *usbdev;
struct ufx_data *dev;
struct fb_info *info;
- int retval;
+ int retval = -ENOMEM;
u32 id_rev, fpga_rev;
/* usb initialization */
@@ -1654,15 +1654,17 @@ static int ufx_usb_probe(struct usb_interface
*interface,
if (!ufx_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
dev_err(dev->gdev, "ufx_alloc_urb_list failed\n");
- goto e_nomem;
+ goto put_ref;
}
/* We don't register a new USB class. Our client interface is fbdev */
/* allocates framebuffer driver structure, not framebuffer memory */
info = framebuffer_alloc(0, >dev);
- if (!info)
- goto e_nomem;
+ if (!info) {
+ dev_err(dev->gdev, "framebuffer_alloc failed\n");
+ goto free_urb_list;
+ }
dev->info = info;
info->par = dev;
@@ -1705,22 +1707,34 @@ static int ufx_usb_probe(struct usb_interface
*interface,
check_warn_goto_error(retval, "unable to find common mode for display and
adapter");
retval = ufx_reg_set_bits(dev, 0x4000, 0x0001);
- check_warn_goto_error(retval, "error %d enabling graphics engine",
retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d enabling graphics engine", retval);
+ goto setup_modes;
+ }
/* ready to begin using device */
atomic_set(>usb_active, 1);
dev_dbg(dev->gdev, "checking var");
retval = ufx_ops_check_var(>var, info);
- check_warn_goto_error(retval, "error %d ufx_ops_check_var", retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d ufx_ops_check_var", retval);
+ goto reset_active;
+ }
dev_dbg(dev->gdev, "setting par");
retval = ufx_ops_set_par(info);
- check_warn_goto_error(retval, "error %d ufx_ops_set_par", retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d ufx_ops_set_par", retval);
+ goto reset_active;
+ }
dev_dbg(dev->gdev, "registering framebuffer");
retval = register_framebuffer(info);
- check_warn_goto_error(retval, "error %d register_framebuffer", retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d register_framebuffer", retval);
+ goto reset_active;
+ }
dev_info(dev->gdev, "SMSC UDX
[PATCH] fbdev: smscufx: fix error handling code in ufx_usb_probe
The current error handling code in ufx_usb_probe have many unmatching
issues, e.g., missing ufx_free_usb_list, destroy_modedb label should
only include framebuffer_release, fb_dealloc_cmap only matches
fb_alloc_cmap.
My local syzkaller reports a memory leak bug:
memory leak in ufx_usb_probe
BUG: memory leak
unreferenced object 0x88802f879580 (size 128):
comm "kworker/0:7", pid 17416, jiffies 4295067474 (age 46.710s)
hex dump (first 32 bytes):
80 21 7c 2e 80 88 ff ff 18 d0 d0 0c 80 88 ff ff .!|.
00 d0 d0 0c 80 88 ff ff e0 ff ff ff 0f 00 00 00
backtrace:
[] kmalloc_trace+0x20/0x90 mm/slab_common.c:1045
[] kmalloc include/linux/slab.h:553 [inline]
[] kzalloc include/linux/slab.h:689 [inline]
[] ufx_alloc_urb_list drivers/video/fbdev/smscufx.c:1873
[inline]
[] ufx_usb_probe+0x11c/0x15a0
drivers/video/fbdev/smscufx.c:1655
[] usb_probe_interface+0x177/0x370
drivers/usb/core/driver.c:396
[] call_driver_probe drivers/base/dd.c:560 [inline]
[] really_probe+0x12d/0x390 drivers/base/dd.c:639
[] __driver_probe_device+0xbf/0x140 drivers/base/dd.c:778
[] driver_probe_device+0x2a/0x120 drivers/base/dd.c:808
[] __device_attach_driver+0xf7/0x150 drivers/base/dd.c:936
[] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
[] __device_attach+0x105/0x2d0 drivers/base/dd.c:1008
[] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
[] device_add+0x642/0xdc0 drivers/base/core.c:3517
[] usb_set_configuration+0x8ef/0xb80
drivers/usb/core/message.c:2170
[] usb_generic_driver_probe+0x8c/0xc0
drivers/usb/core/generic.c:238
[] usb_probe_device+0x5c/0x140
drivers/usb/core/driver.c:293
[] call_driver_probe drivers/base/dd.c:560 [inline]
[] really_probe+0x12d/0x390 drivers/base/dd.c:639
[] __driver_probe_device+0xbf/0x140 drivers/base/dd.c:778
Fix this bug by rewriting the error handling code in ufx_usb_probe.
Reported-by: syzkaller
Tested-by: Dongliang Mu
Signed-off-by: Dongliang Mu
---
drivers/video/fbdev/smscufx.c | 46 +++
1 file changed, 31 insertions(+), 15 deletions(-)
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 9343b7a4ac89..2ad6e98ce10d 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -1622,7 +1622,7 @@ static int ufx_usb_probe(struct usb_interface *interface,
struct usb_device *usbdev;
struct ufx_data *dev;
struct fb_info *info;
- int retval;
+ int retval = -ENOMEM;
u32 id_rev, fpga_rev;
/* usb initialization */
@@ -1654,15 +1654,17 @@ static int ufx_usb_probe(struct usb_interface
*interface,
if (!ufx_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
dev_err(dev->gdev, "ufx_alloc_urb_list failed\n");
- goto e_nomem;
+ goto put_ref;
}
/* We don't register a new USB class. Our client interface is fbdev */
/* allocates framebuffer driver structure, not framebuffer memory */
info = framebuffer_alloc(0, >dev);
- if (!info)
- goto e_nomem;
+ if (!info) {
+ dev_err(dev->gdev, "framebuffer_alloc failed\n");
+ goto free_urb_list;
+ }
dev->info = info;
info->par = dev;
@@ -1705,22 +1707,34 @@ static int ufx_usb_probe(struct usb_interface
*interface,
check_warn_goto_error(retval, "unable to find common mode for display
and adapter");
retval = ufx_reg_set_bits(dev, 0x4000, 0x0001);
- check_warn_goto_error(retval, "error %d enabling graphics engine",
retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d enabling graphics engine", retval);
+ goto setup_modes;
+ }
/* ready to begin using device */
atomic_set(>usb_active, 1);
dev_dbg(dev->gdev, "checking var");
retval = ufx_ops_check_var(>var, info);
- check_warn_goto_error(retval, "error %d ufx_ops_check_var", retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d ufx_ops_check_var", retval);
+ goto reset_active;
+ }
dev_dbg(dev->gdev, "setting par");
retval = ufx_ops_set_par(info);
- check_warn_goto_error(retval, "error %d ufx_ops_set_par", retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d ufx_ops_set_par", retval);
+ goto reset_active;
+ }
dev_dbg(dev->gdev, "registering framebuffer");
retval = register_framebuffer(info);
- check_warn_goto_error(retval, "error %d register_framebuffer", retval);
+ if (retval < 0) {
+ dev_err(dev->gdev, "error %d register_framebuffer", retval);
+ goto reset_active;
+ }
dev_info(dev->gdev, "SMSC UDX USB device /dev/fb%d attached. %dx%d
resolution."
" Using %dK
