linux-kernel - Re: [PATCH] media: zr364xx: fix memory leaks in probe()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Jan 2021 15:20:11 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Antoine Jacquet <royale@...ezo.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-usb@...r.kernel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        syzbot+b4d54814b339b5c6bbd4@...kaller.appspotmail.com
Subject: Re: [PATCH] media: zr364xx: fix memory leaks in probe()

On Wed, Jan 13, 2021 at 05:13:41PM +0100, Hans Verkuil wrote:
> Hi Dan,
> 
> On 06/01/2021 11:10, Dan Carpenter wrote:
> > 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.
> 
> Not a bug, see below.
> 

Thanks for reviewing this.  I will fix a send a v2.  I should have seen
that.

The layering here is sort of confusing in a way...  But not anything
that needs to be dealt with immediately.

regards,
dan carpenter