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]

Message-ID: <20150407144832.GK16501@mwanda>
Date:	Tue, 7 Apr 2015 17:48:32 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Giedrius Statkevičius 
	<giedrius.statkevicius@...il.com>
Cc:	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()

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.

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.

Eventually we will want to clean up dgnc_found_board() error handling
and get rid of brd->dgnc_Major_TransparentPrint_Registered etc.

TODO: dgnc: cleanup dgnc_found_board().

> diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
> index e3564d2..a629a78 100644
> --- a/drivers/staging/dgnc/dgnc_cls.c
> +++ b/drivers/staging/dgnc/dgnc_cls.c
> @@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
>  		return;
>  
>  	ch = brd->channels[port];
> -	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
> +	if (ch->magic != DGNC_CHANNEL_MAGIC)
>  		return;
>  
>  	/* Here we try to figure out what caused the interrupt to happen */
> @@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
>  		/* Loop on each port */
>  		for (i = 0; i < ports; i++) {
>  			ch = bd->channels[i];
> -			if (!ch)
> -				continue;
>  
>  			/*
>  			 * NOTE: Remember you CANNOT hold any channel
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index f5a4d36..1e583c2 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
>  		return;
>  
>  	ch = brd->channels[port];
> -	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
> +	if (ch->magic != DGNC_CHANNEL_MAGIC)
>  		return;
>  

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.

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/