linux-kernel - Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20140529064039.GW17724@mwanda>
Date:	Thu, 29 May 2014 09:40:39 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	DaeSeok Youn <daeseok.youn@...il.com>
Cc:	devel <devel@...verdev.osuosl.org>,
	Lidza Louina <lidza.louina@...il.com>,
	driverdev-devel@...uxdriverproject.org,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Mark Hounschell <markh@...pro.net>
Subject: Re: [PATCH V2] staging: dgap: implement proper error handling in
 dgap_firmware_load()

On Thu, May 29, 2014 at 09:17:09AM +0900, DaeSeok Youn wrote:
> Hi, Dan.
> 
> 2014-05-28 19:11 GMT+09:00 Dan Carpenter <dan.carpenter@...cle.com>:
> > On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote:
> >> > In your patch it has:
> >> > +       dgap_tty_uninit(brd, false);
> >> >
> >> > But it should only be "false" if dgap_tty_init() failed.  If
> >> > dgap_tty_register_ports() fails then it should be "true".  Another
> >> Yes, you're right. There were no error handle for tty_port_register_device() and
> >> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
> >> It need to add error handlers for them, right?
> >
> > Eventually, yes.  But I don't see a simple way to fix
> > dgap_firmware_load() until after the code is cleaned up.
> >
> >>
> >> > problem is that as you say, the earlier function are allocating
> >> > resources like dgap_tty_register() but only the last two function calls
> >> > have a "goto err_cleanup;" so the error handling is incomplete.
> >> So remove "goto" in dgap_firmware_load() and add error handler in
> >> dgap_tty_init()
> >
> > In the current code there isn't a goto in dgap_firmware_load().  Remove
> > the call to dgap_tty_uninit() and add error handling in dgap_tty_init().
> Yes. I will try to fix it.
> >
> > That will clean up the code, and fix some NULL dereference bugs inside
> > dgap_tty_uninit().
> >
> >> and dgap_tty_register_ports(), right?
> >
> > Inside dgap_tty_register_ports(), then we should add a
> > kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails.
> > That is not a complete fix, but it is a part fix and it is clean.
> Actually, I sent a patch which is removing "kfree(brd->serial_ports)" and pushed
> into staging-next branch.
> see the 0ade4a34fd43 staging: dgap: remove unneeded kfree() in
> dgap_tty_register_ports()
> Because I think dgap_tty_uninit() will free "brd->serial_ports" with this patch.
> 
> Can I send a patch after revert "0ade4a34fd43" commit?

Oh, crud.  I missed that.  Yeah.  Let's revert it.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/