linux-kernel - Re: Is this code right in zram?

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: <20120601095330.GC3969@liondog.tnic>
Date:	Fri, 1 Jun 2012 11:53:30 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	John Moser <john.r.moser@...il.com>
Cc:	linux-kernel@...r.kernel.org, Nitin Gupta <ngupta@...are.org>
Subject: Re: Is this code right in zram?

On Thu, May 31, 2012 at 05:43:44PM -0400, John Moser wrote:
> before I go stomping all over other peoples' work and sending
> idiotic patches, I think I'll ask.  Since I have no clue what I'm
> doing.
> 
> in drivers/staging/zram.c out of 3.4

There's no such file in 3.4 - I get

tree v3.4:drivers/staging/zram/

Kconfig
Makefile
zram.txt
zram_drv.c
zram_drv.h
zram_sysfs.c

I'm guessing you mean zram_drv.c - there's code like that below?

> (I just grabbed the source
> hours ago), I see this on lines 810-822:
> 
>        /* Allocate the device array and initialize each one */
>         pr_info("Creating %u devices ...\n", num_devices);
>         zram_devices = kzalloc(num_devices * sizeof(struct zram),
> GFP_KERNEL);
>         if (!zram_devices) {
>                 ret = -ENOMEM;
>                 goto unregister;
>         }
> 
>         for (dev_id = 0; dev_id < num_devices; dev_id++) {
>                 ret = create_device(&zram_devices[dev_id], dev_id);
>                 if (ret)
>                         goto free_devices;
>         }
> 
> Curiosity got me to here:
> 
> http://lwn.net/Articles/147014/
> 
> So assuming this, what I see here is:
> 
>  - kmalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
>  - memset() that to 0
>  - immediately fill in this RAM without reading it
> 
> I'm wondering what the immediate need is to fill the area with
> zeros?

You don't want to use unitialized memory in those zram_devices thingies,
i.e. touch something in there which you haven't initialized before thus
the convention to zero out the whole struct.

Right, create_device() does that init later but what do you do if
create_device changes and forgets one variable, for example? There's
your bug (and a very subtle one, for that matter).

That's why you can't risk it and have to init the memory to a known good
value just in case.

> Also curious as to whether the kzalloc() thing should better be
> kcalloc(num_devices, sizeof(struct zram), GFP_KERNEL) as a matter of
> convention.

That makes sense, let's ask.

-- 
Regards/Gruss,
    Boris.
--
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/