linux-kernel - Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty

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:	Tue, 7 Apr 2015 18:35:51 +0300 (EEST)
From:	Giedrius Statkevičius 
	<giedrius.statkevicius@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
cc:	Giedrius Statkevičius 
	<giedrius.statkevicius@...il.com>, lidza.louina@...il.com,
	markh@...pro.net, gregkh@...uxfoundation.org,
	driverdev-devel@...uxdriverproject.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org, sudipm.mukherjee@...il.com
Subject: Re: [PATCH v3] staging: dgnc: check if kzalloc fails in
 dgnc_tty_init()

On Tue, 7 Apr 2015, Dan Carpenter wrote:

> You will need to update the subject to reflect the new patch.
> 
> The original code did check for kzalloc() failure but it had lots of
> checks scattered around instead nicely at the point where the memory
> was allocated.
> 

There are a lot missing too. For example in dgnc_sysfs.c there are no checks on
any _show() methods if ->channels[i] is NULL or not. And in some other places.

> The old code and the new code are both buggy though and will crash in
> dgnc_tty_uninit().  dgnc_found_board() does "One Err" style error
> handling so it's obviously buggy like the underside of a rock.
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
> 
> It's becoming a difficult thing to fix this because every time we look
> there are more things which don't make sense.
> 
> I believe that if you do:
> 
> > +err_free_channels:
> > +	for (i = i - 1; i >= 0; --i) {
> > +		kfree(brd->channels[i]);
> 		brd->channels[i] = NULL;
> 	}
> > +	return -ENOMEM;
> >  }
> 
> And add some NULL checks in dgnc_tty_uninit() to see if ->channels[i] is
> NULL before doing:
> 
> 	dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
> 
> and
> 	dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
> 
> Then it will fix the bug.
> 

Missed this. Yep, I agree. 

> Do these in a separate patch.  I'm looking for ways we can make this
> patch minimal.  Deleting the comments and the NULL check in
> dgnc_tty_init() is essential for the patch because otherwise the cleanup
> doesn't make sense.

Well, the point of the patch is to put alloc and checks in one place to make the
code less error and bug prone and fix some of bugs where ->channels[i] isn't
checked. Ok, I'll send a v5 that's split into two patches.

> 
> regards,
> dan carpenter
> 
> 

Su pagarba / Regards,
Giedrius
--
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/