linux-kernel - Re: [PATCH] drivers: Conversions from kmalloc+memset to k(z|c)alloc.

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:	Thu, 20 Jul 2006 20:03:12 +0200
From:	Panagiotis Issaris <takis@....org>
To:	Paulo Marques <pmarques@...popie.com>
Cc:	Panagiotis Issaris <takis@...umba.uhasselt.be>,
	linux-kernel@...r.kernel.org, len.brown@...el.com,
	chas@....nrl.navy.mil, miquel@...uba.ar, kkeil@...e.de,
	benh@...nel.crashing.org, video4linux-list@...hat.com,
	rmk+mmc@....linux.org.uk, Neela.Kolli@...enio.com,
	jgarzik@...ox.com, vandrove@...cvut.cz, adaplas@....net,
	thomas@...ischhofer.net, weissg@...nna.at, philb@....org,
	linux-pcmcia@...ts.infradead.org, jkmaline@...hut.fi,
	paulus@...ba.org
Subject: Re: [PATCH] drivers: Conversions from kmalloc+memset to
	k(z|c)alloc.

Hi,

On wo, 2006-07-19 at 14:53 +0100, Paulo Marques wrote:
> > [...]
> > --- a/drivers/char/consolemap.c
> > +++ b/drivers/char/consolemap.c
> > @@ -192,11 +192,9 @@ static void set_inverse_transl(struct vc
> >  	q = p->inverse_translations[i];
> >  
> >  	if (!q) {
> > -		q = p->inverse_translations[i] = (unsigned char *) 
> > -			kmalloc(MAX_GLYPH, GFP_KERNEL);
> > +		q = p->inverse_translations[i] = kzalloc(MAX_GLYPH, GFP_KERNEL);
> >  		if (!q) return;
> >  	}
> > -	memset(q, 0, MAX_GLYPH);
> 
> This changes semantics here. Before, the data pointed by q was always 
> cleared whether it was malloc'ed or not. Now it is only cleared if it is 
> malloc'ed. I haven't checked the code to find out if this is ok, though.
Oops :( My bad. I've removed this from my updated patch. Thanks!

>[...]
> > -			struct cardmap *np = kmalloc(sizeof(*np), GFP_KERNEL);
> > -			memset(np, 0, sizeof(*np));
> > +			struct cardmap *np = kzalloc(sizeof(*np), GFP_KERNEL);
> >  			np->shift = p->shift - CARDMAP_ORDER;
> >  			np->parent = p;
> >  			p->ptr[i] = np;
> 
> This is not your fault, but this code is using the return value from 
> kmalloc (or kzalloc, now) without checking for NULL.
Oops again. I should have noticed that. Thanks again!

> > --- a/drivers/video/offb.c
> > +++ b/drivers/video/offb.c
> > @@ -376,13 +376,12 @@ static void __init offb_init_fb(const ch
> >  
> >  	size = sizeof(struct fb_info) + sizeof(u32) * 17;
> >  
> > -	info = kmalloc(size, GFP_ATOMIC);
> > +	info = kzalloc(size, GFP_ATOMIC);
> >  	
> >  	if (info == 0) {
> 
> Again, not your fault, but "info == 0"? If you're doing a new version of 
> the patch, please change this to NULL or !info, so that we don't confuse 
> human readers :)
Thanks for the hint! I've modified my patch accordingly.

Cheers,
Takis


-
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/