Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp711282pxu; Wed, 6 Jan 2021 02:13:19 -0800 (PST) X-Google-Smtp-Source: ABdhPJxeRCScMuokrLIEdw6CSFSN0isjLgm1UAJB1hpZJau6ZTEk77Sca8DglEDYH3OEWQuEZ+MI X-Received: by 2002:a17:906:354a:: with SMTP id s10mr2363969eja.335.1609927999619; Wed, 06 Jan 2021 02:13:19 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1609927999; cv=none; d=google.com; s=arc-20160816; b=jTeqaE6bkneiRvESw86zfOjaKsgWBFIYhIos1EH2EegXn5/ZxI7Qsf6MCWzarN3Hxz BVVep5ocr5WCXPc4/J55OCGZp4Qoq0nXZpX+4IHGzo2NiZsGccQLq6d2Id/JHHs5CLBe naZ1QDQygmDTcQ19QnCCPiPJrDO9//XEngWkgOpS9FtILwL5Ep4twsPbon3S/qBbgKFy aQZaNHaHJ7XKgZn8HRRMk2S7nfpuW60ISEb9nHEyT8k6ph3WwzU5PWbinGT3QmYF/GA6 ybVTPmSJ1G3d+Px1o7/n8mc50lAi0Dnq7LMz2TuY0cS00xKzLi2xFHYOhpnihd2TQkt4 zsug== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=WYfGnMIIkzNczac23WBRITSIgqNt2y4/Vo4OY/dZozg=; b=fQZ9BFWVBNdXz+047PWp5TB/KAHjVsIr7xYnX/KAPA9quA21MAeBCmnYVqXNrYZPka eoXuRoafStTPLaQgD8FgJnLhsbF3k4/Z65DTW5+APebuAVnvlw5Jv7EC9VMhAGr//3g4 HftD8QbP09Qz6K+arSAoC7NrFGZ8J3dE1kMMcwNFY6iL4OJz0jULJ7vuk+z+TTcuooYy ztDJ6kgwc1ow8yriHcrJcnEsVGT75ZTz48KIlE42dch7qHiCjIKtLW6+P74vfN3GNzjt AX8ykpsmV+T3Or05xaKfIaqk7LIgVTs9rItZaxKWkabIvf+OW5uyo1NYSjDdro2oporA bJIw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=LPqOV4L4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oracle.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id z22si742146ejo.390.2021.01.06.02.12.53; Wed, 06 Jan 2021 02:13:19 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=LPqOV4L4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oracle.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726589AbhAFKLL (ORCPT + 99 others); Wed, 6 Jan 2021 05:11:11 -0500 Received: from aserp2130.oracle.com ([141.146.126.79]:52042 "EHLO aserp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726011AbhAFKLK (ORCPT ); Wed, 6 Jan 2021 05:11:10 -0500 Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 106A4kYH125899; Wed, 6 Jan 2021 10:10:23 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : mime-version : content-type; s=corp-2020-01-29; bh=WYfGnMIIkzNczac23WBRITSIgqNt2y4/Vo4OY/dZozg=; b=LPqOV4L4A9b5R/BRWV1H1iqhXkuuPkZ1zySIn0pBVI4VMkxV717pDikzK437rOmPkxlS EtItT5CQ9HFN2wyUgHvVfo4HtpjouCcDVIfksALC7+YAtHII+94nNkK63Gy7zMj3NVIJ QZ4BKljtfe2vDLR+g8AJ6WZEvsV+l9FaYbRV3MO5dO/E8VRE52VgEHNOuvicMY/si2w9 bq2pdsYM93Mhq1mV+C03ivEs9RviIUPeYsO6hDBQJv9wbEjXV9jK8xoDYSc26uhbnw/C Hq5zJmDLWwEmv18Dc+J9KIWlnRKgu5dypt2D+QIYzVBcbtCljttD1uxVSJl3Fl0P7ljJ Hg== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by aserp2130.oracle.com with ESMTP id 35w7p0grpg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 06 Jan 2021 10:10:23 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 106A6oOu141980; Wed, 6 Jan 2021 10:10:23 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserp3020.oracle.com with ESMTP id 35v1f9qtp4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 06 Jan 2021 10:10:22 +0000 Received: from abhmp0004.oracle.com (abhmp0004.oracle.com [141.146.116.10]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 106AAJQe008541; Wed, 6 Jan 2021 10:10:20 GMT Received: from mwanda (/102.36.221.92) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 06 Jan 2021 02:10:19 -0800 Date: Wed, 6 Jan 2021 13:10:05 +0300 From: Dan Carpenter To: Antoine Jacquet , Andy Shevchenko Cc: Mauro Carvalho Chehab , linux-usb@vger.kernel.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, royale@zerezo.com, syzkaller-bugs , syzbot+b4d54814b339b5c6bbd4@syzkaller.appspotmail.com Subject: [PATCH] media: zr364xx: fix memory leaks in probe() Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Mailer: git-send-email haha only kidding X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9855 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0 suspectscore=0 spamscore=0 bulkscore=0 adultscore=0 mlxscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101060060 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9855 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0 adultscore=0 lowpriorityscore=0 spamscore=0 suspectscore=0 impostorscore=0 mlxscore=0 priorityscore=1501 malwarescore=0 clxscore=1015 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101060060 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Syzbot discovered that the probe error handling doesn't clean up the resources allocated in zr364xx_board_init(). There are several related bugs in this code so I have re-written the error handling. 1) Introduce a new function zr364xx_board_uninit() which cleans up the resources in zr364xx_board_init(). 2) In zr364xx_board_init() if the call to zr364xx_start_readpipe() fails then release the "cam->buffer.frame[i].lpvbits" memory before returning. This way every function either allocates everything successfully or it cleans up after itself. 3) Re-write the probe function so that each failure path goto frees the most recent allocation. That way we don't free anything before it has been allocated and we can also verify that everything is freed. 4) Originally, in the probe function the "cam->v4l2_dev.release" pointer was set to "zr364xx_release" near the start but I moved that assignment to the end, after everything had succeeded. The release function was never actually called during the probe cleanup process, but with this change I wanted to make it clear that we don't want to call zr364xx_release() until everything is allocated successfully. Next I re-wrote the zr364xx_release() function. Ideally this would have been a simple matter of copy and pasting the cleanup code from probe and adding an additional call to video_unregister_device(). But there are several quirks to note. 1) The original code never called video_unregister_device(). In other words, this is an additional bugfix. 2) The probe function does not call videobuf_mmap_free() and I don't know where the videobuf_mmap is allocated. I left the code as-is to avoid introducing a bug in code I don't understand. 3) The zr364xx_board_uninit() has a call to zr364xx_stop_readpipe() which is a change from the original behavior with regards to unloading the driver. Calling zr364xx_stop_readpipe() on a stopped pipe is not a problem so this is safe and is potentially a bugfix. Reported-by: syzbot+b4d54814b339b5c6bbd4@syzkaller.appspotmail.com Signed-off-by: Dan Carpenter --- drivers/media/usb/zr364xx/zr364xx.c | 50 ++++++++++++++++++----------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c index 1e1c6b4d1874..62a232f995a7 100644 --- a/drivers/media/usb/zr364xx/zr364xx.c +++ b/drivers/media/usb/zr364xx/zr364xx.c @@ -1181,15 +1181,11 @@ static int zr364xx_open(struct file *file) return err; } -static void zr364xx_release(struct v4l2_device *v4l2_dev) +static void zr364xx_board_uninit(struct zr364xx_camera *cam) { - struct zr364xx_camera *cam = - container_of(v4l2_dev, struct zr364xx_camera, v4l2_dev); unsigned long i; - v4l2_device_unregister(&cam->v4l2_dev); - - videobuf_mmap_free(&cam->vb_vidq); + zr364xx_stop_readpipe(cam); /* release sys buffers */ for (i = 0; i < FRAMES; i++) { @@ -1200,9 +1196,20 @@ static void zr364xx_release(struct v4l2_device *v4l2_dev) cam->buffer.frame[i].lpvbits = NULL; } - v4l2_ctrl_handler_free(&cam->ctrl_handler); /* release transfer buffer */ kfree(cam->pipe->transfer_buffer); +} + +static void zr364xx_release(struct v4l2_device *v4l2_dev) +{ + struct zr364xx_camera *cam = + container_of(v4l2_dev, struct zr364xx_camera, v4l2_dev); + + videobuf_mmap_free(&cam->vb_vidq); + video_unregister_device(&cam->vdev); + v4l2_ctrl_handler_free(&cam->ctrl_handler); + zr364xx_board_uninit(cam); + v4l2_device_unregister(&cam->v4l2_dev); kfree(cam); } @@ -1376,11 +1383,14 @@ static int zr364xx_board_init(struct zr364xx_camera *cam) /* start read pipe */ err = zr364xx_start_readpipe(cam); if (err) - goto err_free; + goto err_free_frames; DBG(": board initialized\n"); return 0; +err_free_frames: + for (i = 0; i < FRAMES; i++) + vfree(cam->buffer.frame[i].lpvbits); err_free: kfree(cam->pipe->transfer_buffer); cam->pipe->transfer_buffer = NULL; @@ -1409,12 +1419,10 @@ static int zr364xx_probe(struct usb_interface *intf, if (!cam) return -ENOMEM; - cam->v4l2_dev.release = zr364xx_release; err = v4l2_device_register(&intf->dev, &cam->v4l2_dev); if (err < 0) { dev_err(&udev->dev, "couldn't register v4l2_device\n"); - kfree(cam); - return err; + goto free_cam; } hdl = &cam->ctrl_handler; v4l2_ctrl_handler_init(hdl, 1); @@ -1423,7 +1431,7 @@ static int zr364xx_probe(struct usb_interface *intf, if (hdl->error) { err = hdl->error; dev_err(&udev->dev, "couldn't register control\n"); - goto fail; + goto unregister; } /* save the init method used by this camera */ cam->method = id->driver_info; @@ -1496,7 +1504,7 @@ static int zr364xx_probe(struct usb_interface *intf, if (!cam->read_endpoint) { err = -ENOMEM; dev_err(&intf->dev, "Could not find bulk-in endpoint\n"); - goto fail; + goto unregister; } /* v4l */ @@ -1507,10 +1515,11 @@ static int zr364xx_probe(struct usb_interface *intf, /* load zr364xx board specific */ err = zr364xx_board_init(cam); - if (!err) - err = v4l2_ctrl_handler_setup(hdl); if (err) - goto fail; + goto unregister; + err = v4l2_ctrl_handler_setup(hdl); + if (err) + goto board_uninit; spin_lock_init(&cam->slock); @@ -1525,16 +1534,21 @@ static int zr364xx_probe(struct usb_interface *intf, err = video_register_device(&cam->vdev, VFL_TYPE_VIDEO, -1); if (err) { dev_err(&udev->dev, "video_register_device failed\n"); - goto fail; + goto free_handler; } + cam->v4l2_dev.release = zr364xx_release; dev_info(&udev->dev, DRIVER_DESC " controlling device %s\n", video_device_node_name(&cam->vdev)); return 0; -fail: +free_handler: v4l2_ctrl_handler_free(hdl); +board_uninit: + zr364xx_board_uninit(cam); +unregister: v4l2_device_unregister(&cam->v4l2_dev); +free_cam: kfree(cam); return err; } -- 2.29.2