[U-Boot] [PATCH 1/2] net: Remove call of srand from eth_random_enetaddr()

[U-Boot] [PATCH 1/2] net: Remove call of srand from eth_random_enetaddr()

[Christian Riesch]

Currently eth_random_enetaddr() seeds the random number generator with
get_timer(0). Some boards might want to use other sources for the seed,
therefore move the call of srand() to the board specific code.

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
Cc: Michael Walle <michael@walle.cc>
Cc: Joe Hershberger <joe.hershberger@gmail.com>
---
 board/buffalo/lsxl/lsxl.c |    1 +
 include/net.h             |    3 +++
 net/eth.c                 |    2 --
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/board/buffalo/lsxl/lsxl.c b/board/buffalo/lsxl/lsxl.c
index 57776fb..b7eb0dc 100644
--- a/board/buffalo/lsxl/lsxl.c
+++ b/board/buffalo/lsxl/lsxl.c
@@ -248,6 +248,7 @@ static void rescue_mode(void)
 	printf("Entering rescue mode..\n");
 #ifdef CONFIG_RANDOM_MACADDR
 	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
+		srand(get_timer(0));
 		eth_random_enetaddr(enetaddr);
 		if (eth_setenv_enetaddr("ethaddr", enetaddr)) {
 			printf("Failed to set ethernet address\n");
diff --git a/include/net.h b/include/net.h
index 970d4d1..5fc3693 100644
--- a/include/net.h
+++ b/include/net.h
@@ -141,6 +141,9 @@ extern int eth_getenv_enetaddr_by_index(const char *base_name, int index,
  *
  * In these cases, we generate a random locally administered ethernet address.
  *
+ * Remember to seed the random number generator with srand() before calling
+ * this functon.
+ *
  * Args:
  *  enetaddr - returns 6 byte hardware address
  */
diff --git a/net/eth.c b/net/eth.c
index 321d5b1..dc4cc20 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -84,8 +84,6 @@ void eth_random_enetaddr(uchar *enetaddr)
 {
 	uint32_t rval;
 
-	srand(get_timer(0));
-
 	rval = rand();
 	enetaddr[0] = rval & 0xff;
 	enetaddr[1] = (rval >> 8) & 0xff;
-- 
1.7.9.5

[U-Boot] [PATCH 1/2] net: Remove call of srand from eth_random_enetaddr()

[Wolfgang Denk]

Dear Christian Riesch,

In message <419e5c6e-b2ef-44c2-a4c1-bb25c50fcb57@mary.at.omicron.at> you wrote:
> Currently eth_random_enetaddr() seeds the random number generator with
> get_timer(0). Some boards might want to use other sources for the seed,
> therefore move the call of srand() to the board specific code.
> 
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>

I don't like this change.  What exactly is wrong with using the timer
here?   It is probably much more random that the (so-called)
"un-initialized" memory you suggest to use instead.

If there is really need to use another inital valu, only this should
be fixed - but the srand() call itself should remain as is.


You cannot do this.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You can observe a lot just by watchin'.                  - Yogi Berra

[U-Boot] [PATCH 1/2] net: Remove call of srand from eth_random_enetaddr()

[Christian Riesch]

Hello Wolfgang,
Thank you for your comments.

On Tuesday, January 8, 2013, Wolfgang Denk wrote:

> Dear Christian Riesch,
>
> In message <419e5c6e-b2ef-44c2-a4c1-bb25c50fcb57@mary.at.omicron.at<javascript:;>>
> you wrote:
> > Currently eth_random_enetaddr() seeds the random number generator with
> > get_timer(0). Some boards might want to use other sources for the seed,
> > therefore move the call of srand() to the board specific code.
> >
> > Signed-off-by: Christian Riesch <christian.riesch@omicron.at<javascript:;>
> >
> > Cc: Michael Walle <michael@walle.cc>
> > Cc: Joe Hershberger <joe.hershberger@gmail.com <javascript:;>>
>
> I don't like this change.  What exactly is wrong with using the timer
> here?   It is probably much more random that the (so-called)
> "un-initialized" memory you suggest to use instead.


On the AM1808 SoC the counter is reset to zero on power up. So using it to
generate random numbers in code that is called interactively by the user is
fine and will yield a random MAC address, but in my case I will get the
same MAC address on each board at each power up.


> If there is really need to use another inital valu, only this should
> be fixed - but the srand() call itself should remain as is.
>
>
>
For other boards it may be ok to use a counter, and for some there may be
no SRAM or it may be already overwritten, e.g by the SPL... Therefore I am
not changing this for all boards, but make it board specific.

Regards, Christian


> You cannot do this.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de<javascript:;>
> You can observe a lot just by watchin'.                  - Yogi Berra
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de <javascript:;>
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH 1/2] net: Remove call of srand from eth_random_enetaddr()

[Wolfgang Denk]

Dear Christian Riesch,

In message <CABkLObrL2BhKmjh9NCcv1UQ-cbfJisUidK8JoE8DSAOEkqiZ1g@mail.gmail.com> you wrote:
>
> > I don't like this change.  What exactly is wrong with using the timer
> > here?   It is probably much more random that the (so-called)
> > "un-initialized" memory you suggest to use instead.
> 
> On the AM1808 SoC the counter is reset to zero on power up. So using it to
> generate random numbers in code that is called interactively by the user is
> fine and will yield a random MAC address, but in my case I will get the
> same MAC address on each board at each power up.

I think the whole concept of using random MAC adresses is broken.  You
should consider thinking about fixing the root cause of your problem.

> > If there is really need to use another inital valu, only this should
> > be fixed - but the srand() call itself should remain as is.
>
> For other boards it may be ok to use a counter, and for some there may be
> no SRAM or it may be already overwritten, e.g by the SPL... Therefore I am
> not changing this for all boards, but make it board specific.

I don't want to see any board specific code here.  If using the timer
is not good enough, then pass a weak funtion as argument to srand()
which defaults to using the timer as we do now, and which can be
redefined in board specific code.

Other than this, the common code should not be changed.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The main thing is the play itself. I swear that greed for  money  has
nothing  to  do with it, although heaven knows I am sorely in need of
money.                                           - Feodor Dostoyevsky

[U-Boot] [PATCH 2/2] calimain: Generate random MAC address for factory tests

[Christian Riesch]

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
---
 board/omicron/calimain/calimain.c |   31 ++++++++++++++++++++++++++++++-
 include/configs/calimain.h        |    2 ++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/board/omicron/calimain/calimain.c b/board/omicron/calimain/calimain.c
index 1060a1f..80e3893 100644
--- a/board/omicron/calimain/calimain.c
+++ b/board/omicron/calimain/calimain.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011 OMICRON electronics GmbH
+ * Copyright (C) 2011-2013 OMICRON electronics GmbH
  *
  * Based on da850evm.c. Original Copyrights follow:
  *
@@ -136,6 +136,35 @@ int board_init(void)
 	return 0;
 }
 
+/* seed random number generator with uninitialized SRAM content */
+static void srand_sram(void)
+{
+	int *p;
+	int seed = 0;
+
+	for (p = (int *) 0x80000000; p < (int *) 0x8001ffff; p++)
+		seed ^= *p;
+
+	srand(seed);
+}
+
+int board_late_init(void)
+{
+	uchar enetaddr[6];
+
+	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
+		srand_sram();
+		eth_random_enetaddr(enetaddr);
+		if (eth_setenv_enetaddr("ethaddr", enetaddr)) {
+			printf("Failed to set random ethernet address\n");
+		} else {
+			printf("Setting random ethernet address %pM.\n",
+			       enetaddr);
+		}
+	}
+	return 0;
+}
+
 #ifdef CONFIG_DRIVER_TI_EMAC
 /*
  * Initializes on-board ethernet controllers.
diff --git a/include/configs/calimain.h b/include/configs/calimain.h
index 5c2b35d..8cea0d9 100644
--- a/include/configs/calimain.h
+++ b/include/configs/calimain.h
@@ -30,6 +30,7 @@
 #define CONFIG_DRIVER_TI_EMAC
 #define MACH_TYPE_CALIMAIN	3528
 #define CONFIG_MACH_TYPE	MACH_TYPE_CALIMAIN
+#define CONFIG_BOARD_LATE_INIT
 
 /*
  * SoC Configuration
@@ -202,6 +203,7 @@
 #define CONFIG_BOOTP_DNS2
 #define CONFIG_BOOTP_SEND_HOSTNAME
 #define CONFIG_NET_RETRY_COUNT	10
+#define CONFIG_RANDOM_MACADDR
 #endif
 
 /*
-- 
1.7.9.5

[U-Boot] [PATCH 2/2] calimain: Generate random MAC address for factory tests

[Wolfgang Denk]

Dear Christian Riesch,

In message <6cc4810c-1e2e-4ebf-912a-96936f035c0e@mary.at.omicron.at> you wrote:
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> ---
>  board/omicron/calimain/calimain.c |   31 ++++++++++++++++++++++++++++++-
>  include/configs/calimain.h        |    2 ++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/board/omicron/calimain/calimain.c b/board/omicron/calimain/calimain.c
> index 1060a1f..80e3893 100644
> --- a/board/omicron/calimain/calimain.c
> +++ b/board/omicron/calimain/calimain.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2011 OMICRON electronics GmbH
> + * Copyright (C) 2011-2013 OMICRON electronics GmbH
>   *
>   * Based on da850evm.c. Original Copyrights follow:
>   *
> @@ -136,6 +136,35 @@ int board_init(void)
>  	return 0;
>  }
>  
> +/* seed random number generator with uninitialized SRAM content */
> +static void srand_sram(void)
> +{
> +	int *p;
> +	int seed = 0;
> +
> +	for (p = (int *) 0x80000000; p < (int *) 0x8001ffff; p++)
> +		seed ^= *p;
> +
> +	srand(seed);
> +}

Note that your "uninitialized" SRAM content is probably not so much
random at all - I guess, it is much less random than the originally
used timer value.

What exactly is your justification for such a change?  Please
elucidate...

> +int board_late_init(void)
> +{
> +	uchar enetaddr[6];
> +
> +	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
> +		srand_sram();
> +		eth_random_enetaddr(enetaddr);
> +		if (eth_setenv_enetaddr("ethaddr", enetaddr)) {
> +			printf("Failed to set random ethernet address\n");
> +		} else {
> +			printf("Setting random ethernet address %pM.\n",
> +			       enetaddr);
> +		}
> +	}
> +	return 0;
> +}

NAK! You are but duplicating the code already present in net/eth.c

This makes no sense.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is dangerous to be right on a subject  on  which  the  established
authorities are wrong.                                    -- Voltaire

[U-Boot] [PATCH 2/2] calimain: Generate random MAC address for factory tests

[Christian Riesch]

Hello Wolfgang,
Thank you again for your comments.

On 2013-01-08 18:39, Wolfgang Denk wrote:
> Dear Christian Riesch,
>
> In message <6cc4810c-1e2e-4ebf-912a-96936f035c0e@mary.at.omicron.at> you wrote:
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> ---
>>   board/omicron/calimain/calimain.c |   31 ++++++++++++++++++++++++++++++-
>>   include/configs/calimain.h        |    2 ++
>>   2 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/board/omicron/calimain/calimain.c b/board/omicron/calimain/calimain.c
>> index 1060a1f..80e3893 100644
>> --- a/board/omicron/calimain/calimain.c
>> +++ b/board/omicron/calimain/calimain.c
>> @@ -1,5 +1,5 @@
>>   /*
>> - * Copyright (C) 2011 OMICRON electronics GmbH
>> + * Copyright (C) 2011-2013 OMICRON electronics GmbH
>>    *
>>    * Based on da850evm.c. Original Copyrights follow:
>>    *
>> @@ -136,6 +136,35 @@ int board_init(void)
>>   	return 0;
>>   }
>>
>> +/* seed random number generator with uninitialized SRAM content */
>> +static void srand_sram(void)
>> +{
>> +	int *p;
>> +	int seed = 0;
>> +
>> +	for (p = (int *) 0x80000000; p < (int *) 0x8001ffff; p++)
>> +		seed ^= *p;
>> +
>> +	srand(seed);
>> +}
>
> Note that your "uninitialized" SRAM content is probably not so much
> random at all -

There are several papers around describing the use of initial SRAM value 
after power up for the generation of random numbers. This is why I gave 
it a try, and it works pretty well for me. I get a different seed for 
each power-up cycle. I guess that the randomness is limited and that 
part of the generated seed is more a fingerprint for the chip, therefore 
it may not be good enough for security related stuff, but for my purpose 
it's ok.

> I guess, it is much less random than the originally
> used timer value.

In my case the timer value is not random at all since it is reset to 
zero at power-up. Since seeding the random number generator is always 
done at the same time after power-up in the current code, the seed will 
always be the same for my devices. Therefore the generated MAC address 
will always be the same for all devices.

> What exactly is your justification for such a change?  Please
> elucidate...

Actually I do not change anything ;-)

For the lsxl board that is currently the only user of eth_random_enet(), 
nothing changes. get_timer(0) remains the source of the randomness for 
this board. My patches only allow other boards to use a sources of 
randomness that is available to them instead of forcing everyone to use 
get_timer(0).

>
>> +int board_late_init(void)
>> +{
>> +	uchar enetaddr[6];
>> +
>> +	if (!eth_getenv_enetaddr("ethaddr", enetaddr)) {
>> +		srand_sram();
>> +		eth_random_enetaddr(enetaddr);
>> +		if (eth_setenv_enetaddr("ethaddr", enetaddr)) {
>> +			printf("Failed to set random ethernet address\n");
>> +		} else {
>> +			printf("Setting random ethernet address %pM.\n",
>> +			       enetaddr);
>> +		}
>> +	}
>> +	return 0;
>> +}
>
> NAK! You are but duplicating the code already present in net/eth.c

Apparently I am missing something here. I do not see a call of 
eth_random_enetaddr() in net/eth.c. To which part of net/eth.c are you 
referring?

Regards, Christian

> This makes no sense.
>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [PATCH 2/2] calimain: Generate random MAC address for factory tests

[Wolfgang Denk]

Dear Christian Riesch,

In message <50ED2B84.2080905@omicron.at> you wrote:
> 
> > What exactly is your justification for such a change?  Please
> > elucidate...
> 
> Actually I do not change anything ;-)

Why do you need a patch then? :-P

> > NAK! You are but duplicating the code already present in net/eth.c
> 
> Apparently I am missing something here. I do not see a call of 
> eth_random_enetaddr() in net/eth.c. To which part of net/eth.c are you 
> referring?

Hm... I can't find it now, either. Dunno what I have seen then.
eventually I was looking at your patch in two windows :-(

Sorry - but still: very similar code exists in rescue_mode() in
"board/buffalo/lsxl/lsxl.c"; this should be factored out.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Any sufficiently advanced  technology  is  indistinguishable  from  a
rigged demo.                              - Andy Finkel, computer guy