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 0xffff88802f879580 (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:
[<ffffffff814c99a0>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1045
[<ffffffff824d219c>] kmalloc include/linux/slab.h:553 [inline]
[<ffffffff824d219c>] kzalloc include/linux/slab.h:689 [inline]
[<ffffffff824d219c>] ufx_alloc_urb_list drivers/video/fbdev/smscufx.c:1873 [inline]
[<ffffffff824d219c>] ufx_usb_probe+0x11c/0x15a0 drivers/video/fbdev/smscufx.c:1655
[<ffffffff82d17927>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
[<ffffffff82712f0d>] call_driver_probe drivers/base/dd.c:560 [inline]
[<ffffffff82712f0d>] really_probe+0x12d/0x390 drivers/base/dd.c:639
[<ffffffff8271322f>] __driver_probe_device+0xbf/0x140 drivers/base/dd.c:778
[<ffffffff827132da>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:808
[<ffffffff82713c27>] __device_attach_driver+0xf7/0x150 drivers/base/dd.c:936
[<ffffffff82710137>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
[<ffffffff827136b5>] __device_attach+0x105/0x2d0 drivers/base/dd.c:1008
[<ffffffff82711d36>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
[<ffffffff8270e242>] device_add+0x642/0xdc0 drivers/base/core.c:3517
[<ffffffff82d14d5f>] usb_set_configuration+0x8ef/0xb80 drivers/usb/core/message.c:2170
[<ffffffff82d2576c>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238
[<ffffffff82d16ffc>] usb_probe_device+0x5c/0x140 drivers/usb/core/driver.c:293
[<ffffffff82712f0d>] call_driver_probe drivers/base/dd.c:560 [inline]
[<ffffffff82712f0d>] really_probe+0x12d/0x390 drivers/base/dd.c:639
[<ffffffff8271322f>] __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 <[email protected]>
Tested-by: Dongliang Mu <[email protected]>
Signed-off-by: Dongliang Mu <[email protected]>
---
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, &usbdev->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, 0x00000001);
- 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(&dev->usb_active, 1);
dev_dbg(dev->gdev, "checking var");
retval = ufx_ops_check_var(&info->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 framebuffer memory\n", info->node,
@@ -1728,21 +1742,23 @@ static int ufx_usb_probe(struct usb_interface *interface,
return 0;
-error:
- fb_dealloc_cmap(&info->cmap);
-destroy_modedb:
+reset_active:
+ atomic_set(&dev->usb_active, 0);
+setup_modes:
fb_destroy_modedb(info->monspecs.modedb);
vfree(info->screen_base);
fb_destroy_modelist(&info->modelist);
+error:
+ fb_dealloc_cmap(&info->cmap);
+destroy_modedb:
framebuffer_release(info);
+free_urb_list:
+ if (dev->urbs.count > 0)
+ ufx_free_urb_list(dev);
put_ref:
kref_put(&dev->kref, ufx_free); /* ref for framebuffer */
kref_put(&dev->kref, ufx_free); /* last ref from kref_init */
return retval;
-
-e_nomem:
- retval = -ENOMEM;
- goto put_ref;
}
static void ufx_usb_disconnect(struct usb_interface *interface)
--
2.34.1
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 0xffff88802f879580 (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:
> [<ffffffff814c99a0>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1045
> [<ffffffff824d219c>] kmalloc include/linux/slab.h:553 [inline]
> [<ffffffff824d219c>] kzalloc include/linux/slab.h:689 [inline]
> [<ffffffff824d219c>] ufx_alloc_urb_list drivers/video/fbdev/smscufx.c:1873 [inline]
> [<ffffffff824d219c>] ufx_usb_probe+0x11c/0x15a0 drivers/video/fbdev/smscufx.c:1655
> [<ffffffff82d17927>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
> [<ffffffff82712f0d>] call_driver_probe drivers/base/dd.c:560 [inline]
> [<ffffffff82712f0d>] really_probe+0x12d/0x390 drivers/base/dd.c:639
> [<ffffffff8271322f>] __driver_probe_device+0xbf/0x140 drivers/base/dd.c:778
> [<ffffffff827132da>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:808
> [<ffffffff82713c27>] __device_attach_driver+0xf7/0x150 drivers/base/dd.c:936
> [<ffffffff82710137>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
> [<ffffffff827136b5>] __device_attach+0x105/0x2d0 drivers/base/dd.c:1008
> [<ffffffff82711d36>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
> [<ffffffff8270e242>] device_add+0x642/0xdc0 drivers/base/core.c:3517
> [<ffffffff82d14d5f>] usb_set_configuration+0x8ef/0xb80 drivers/usb/core/message.c:2170
> [<ffffffff82d2576c>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238
> [<ffffffff82d16ffc>] usb_probe_device+0x5c/0x140 drivers/usb/core/driver.c:293
> [<ffffffff82712f0d>] call_driver_probe drivers/base/dd.c:560 [inline]
> [<ffffffff82712f0d>] really_probe+0x12d/0x390 drivers/base/dd.c:639
> [<ffffffff8271322f>] __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 <[email protected]>
> Tested-by: Dongliang Mu <[email protected]>
> Signed-off-by: Dongliang Mu <[email protected]>
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, &usbdev->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, 0x00000001);
> - 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(&dev->usb_active, 1);
>
> dev_dbg(dev->gdev, "checking var");
> retval = ufx_ops_check_var(&info->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 framebuffer memory\n", info->node,
> @@ -1728,21 +1742,23 @@ static int ufx_usb_probe(struct usb_interface *interface,
>
> return 0;
>
> -error:
> - fb_dealloc_cmap(&info->cmap);
> -destroy_modedb:
> +reset_active:
> + atomic_set(&dev->usb_active, 0);
> +setup_modes:
> fb_destroy_modedb(info->monspecs.modedb);
> vfree(info->screen_base);
> fb_destroy_modelist(&info->modelist);
> +error:
> + fb_dealloc_cmap(&info->cmap);
> +destroy_modedb:
> framebuffer_release(info);
> +free_urb_list:
> + if (dev->urbs.count > 0)
> + ufx_free_urb_list(dev);
> put_ref:
> kref_put(&dev->kref, ufx_free); /* ref for framebuffer */
> kref_put(&dev->kref, ufx_free); /* last ref from kref_init */
> return retval;
> -
> -e_nomem:
> - retval = -ENOMEM;
> - goto put_ref;
> }
>
> static void ufx_usb_disconnect(struct usb_interface *interface)