linux-kernel - Re: [PATCH 1/3] crypto: hw_random

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:   Fri, 24 Mar 2017 17:43:48 +0300
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Stephan Müller <smueller@...onox.de>
Cc:     Kukjin Kim <kgene@...nel.org>,
        Javier Martinez Canillas <javier@....samsung.com>,
        Matt Mackall <mpm@...enic.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-crypto@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>, Olof Johansson <olof@...om.net>
Subject: Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

On Fri, Mar 24, 2017 at 03:37:59PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 15:24:44 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > +
> > +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> > +			       const u8 *seed, unsigned int slen)
> > +{
> > +	int ret, i;
> > +	u32 val;
> > +
> > +	dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> > +
> > +	ret = clk_prepare_enable(rng->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (slen < EXYNOS_RNG_SEED_SIZE) {
> > +		dev_warn(rng->dev, "Seed too short (only %u bytes)\n", slen);
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> > +		val = seed[i * 4] << 24;
> > +		val |= seed[i * 4 + 1] << 16;
> > +		val |= seed[i * 4 + 2] << 8;
> > +		val |= seed[i * 4 + 3] << 0;
> > +
> > +		exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> > +	}
> > +
> > +	val = exynos_rng_readl(rng, EXYNOS_RNG_STATUS);
> > +	if (!(val & EXYNOS_RNG_STATUS_SEED_SETTING_DONE)) {
> > +		dev_warn(rng->dev, "Seed setting not finished\n");
> > +		ret = -EIO;
> > +		goto out;
> > +	}
> > +
> > +	ret = 0;
> > +	/* Save seed for suspend */
> > +	memcpy(rng->seed_save, seed, slen);
> 
> Is this really necessary? If you need to save some seed, shouldn't that be an 
> output of the DRNG and not the real seed?

When suspended to RAM device will loose the contents of registers
(including SEED registers) so it has to be initialized with something on
resume.

The seed registers are write-only so I cannot read them and store
contents just before suspend.

I understand that real seed should not be stored... but then if I am not
able to re-seed it with same values, I will loose the continuous and
reproducible pseudo-random generation after suspend. Aren't this
expected out of PRNG?

> Besides, how do you know that slen is not larger than EXYNOS_RNG_SEED_SIZE?

Right, there is a overflow here. It should be sizeof(rng->seed_save);

Thanks for review!

Best regards,
Krzysztof