linux-kernel - Re: [PATCH] hwrng: Add TX4939 RNG driver

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: <20090601.014517.169682203.anemo@mba.ocn.ne.jp>
Date:	Mon, 01 Jun 2009 01:45:17 +0900 (JST)
From:	Atsushi Nemoto <anemo@....ocn.ne.jp>
To:	akpm@...ux-foundation.org
Cc:	linux-mips@...ux-mips.org, ralf@...ux-mips.org,
	linux-kernel@...r.kernel.org, herbert@...dor.apana.org.au,
	mpm@...enic.com
Subject: Re: [PATCH] hwrng: Add TX4939 RNG driver

On Fri, 29 May 2009 16:29:07 -0700, Andrew Morton <akpm@...ux-foundation.org> wrote:
> I assume that the MIPS patch "[PATCH] TXx9: Add TX4939 RNG support"
> depends upon this patch?

To build kernel or driver, no dependencies.  To use this device
actually, both patches are needed.

> > +static u64 read_rng(void __iomem *base, unsigned int offset)
> > +{
> > +	/* Caller must disable interrupts */
> > +	return ____raw_readq(base + offset);
> > +}
> 
> What is the reasoning behind the local_irq_disable() requirement?
> 
> Because I'm wondering whether this is safe on SMP?

As Ralf replied, These local_irq_disable stuff are just for 64-bit
access on 32-bit kernel.  Maybe something like this is preferred?

static void ____raw_io_start(void)
{
#ifndef CONFIG_64BIT
	/* some comments... */
	local_irq_enable();
#endif
}

static void ____raw_io_end(void)
{
#ifndef CONFIG_64BIT
	/* see above */
	local_irq_disable();
#endif
}

For SMP concurrent access, these rountines are protected by mutex in
rng-core.  Also this SoC does not support SMP.  There should be no
problem here.

> > +	for (i = 0; i < 20; i++) {
...
> > +		udelay(1);
> > +	}
> > +	return rngdev->data_avail;
> > +}
> 
> The mysterious udelay() needs a comment, because there is no way in
> which the reader can otherwise work out why it is there.

Well, this comments can be applied most RNG drivers ;)

Anyway, I will add some comment here.  I take this loop (20 loops with
udelay) from other drivers and changed to udelay(1) because the
datasheed states "90 bus clock cycles by default" for generation
(typically 450ns for this SoC).

> > +static int tx4939_rng_data_read(struct hwrng *rng, u32 *buffer)
> > +{
> > +	struct tx4939_rng *rngdev = container_of(rng, struct tx4939_rng, rng);
> > +
> > +	rngdev->data_avail--;
> > +	*buffer = *((u32 *)&rngdev->databuf + rngdev->data_avail);
> > +	return sizeof(u32);
> > +}
> 
> Concurrent callers can corrupt rngdev->data_avail ?

This is protected by rng_mutex in rng-core.

> > +	/* Start RNG */
> > +	write_rng(TX4939_RNG_RCSR_ST, rngdev->base, TX4939_RNG_RCSR);
> > +	local_irq_enable();
> > +	/* drop first two results */
> 
> The comment doesn't provide the reason for doing this?

>From the datasheet:

	The quality of the random numbers generated immediately after
	reset can be insufficient.  Therefore, do not use random
	numbers obtained from the first and second generations; use
	the ones from the third or subsequent generation.

I will put this comment in the driver.

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