2010-12-31 00:37:35

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Thu, Dec 30, 2010 at 04:49:05PM -0600, Larry Finger wrote:
>
> Do you see any problems in the code in drivers/net/wireless/b43/main.c or
> drivers/char/hw_random/via-rng.c. As the latter seems to make b43 fail, I am
> suspecting via-rng, but I have no proof.

My suspicion is that VIA's xstore is writing more than 4 bytes as
the list pointer happens to lie immediately after rng->priv which
is where xstore is writing to.

Harald, do you know whether this is documented or is this a known
errata item?

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


2010-12-31 00:46:31

by Larry Finger

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On 12/30/2010 06:37 PM, Herbert Xu wrote:
> On Thu, Dec 30, 2010 at 04:49:05PM -0600, Larry Finger wrote:
>>
>> Do you see any problems in the code in drivers/net/wireless/b43/main.c or
>> drivers/char/hw_random/via-rng.c. As the latter seems to make b43 fail, I am
>> suspecting via-rng, but I have no proof.
>
> My suspicion is that VIA's xstore is writing more than 4 bytes as
> the list pointer happens to lie immediately after rng->priv which
> is where xstore is writing to.
>
> Harald, do you know whether this is documented or is this a known
> errata item?

The following patch should be able to test if xstore is overwriting the list
pointer.

Larry
---

Index: wireless-testing/include/linux/hw_random.h
===================================================================
--- wireless-testing.orig/include/linux/hw_random.h
+++ wireless-testing/include/linux/hw_random.h
@@ -38,6 +38,7 @@ struct hwrng {
int (*data_read)(struct hwrng *rng, u32 *data);
int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
unsigned long priv;
+ char junk[12];

/* internal. */
struct list_head list;



Attachments:
hwrng_debug (454.00 B)

2010-12-31 02:29:21

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Thu, Dec 30, 2010 at 06:46:31PM -0600, Larry Finger wrote:
> On 12/30/2010 06:37 PM, Herbert Xu wrote:
> > My suspicion is that VIA's xstore is writing more than 4 bytes as
> > the list pointer happens to lie immediately after rng->priv which
> > is where xstore is writing to.
> >
> > Harald, do you know whether this is documented or is this a known
> > errata item?
>
> The following patch should be able to test if xstore is overwriting the list
> pointer.

Confirmed. No crashes with the junk buffer in action.
I applied both patches (dump_stack() in hwrng_register() and junk[]
after priv data) to vanilla 2.6.37-rc7 and tested both: via-rng and my
via+rng2 as well as via-rng and b43-rng - no crashes. The (previously
also crashing) `cat rng_available' does survive as well:

$ cat /sys/devices/virtual/misc/hw_random/rng_available
via b43_phy0 via2
$

Attached 2 dmesg excerpts.


regards & g'nite
Mario
--
Tower: "Say fuelstate." Pilot: "Fuelstate."
Tower: "Say again." Pilot: "Again."
Tower: "Arghl, give me your fuel!" Pilot: "Sorry, need it by myself..."


Attachments:
(No filename) (0.00 B)
signature.asc (482.00 B)
Digital signature
Download all attachments

2010-12-31 02:47:03

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Fri, Dec 31, 2010 at 03:25:51AM +0100, Mario 'BitKoenig' Holbe wrote:
>
> Confirmed. No crashes with the junk buffer in action.
> I applied both patches (dump_stack() in hwrng_register() and junk[]
> after priv data) to vanilla 2.6.37-rc7 and tested both: via-rng and my
> via+rng2 as well as via-rng and b43-rng - no crashes. The (previously
> also crashing) `cat rng_available' does survive as well:
>
> $ cat /sys/devices/virtual/misc/hw_random/rng_available
> via b43_phy0 via2
> $
>
> Attached 2 dmesg excerpts.

Thanks for checking. Can you provide the output of

cat /proc/cpuinfo

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2010-12-31 08:51:23

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Fri, Dec 31, 2010 at 01:46:53PM +1100, Herbert Xu wrote:
> Thanks for checking. Can you provide the output of
> cat /proc/cpuinfo

attached.


Mario
--
The only thing to be scared of, son, is tomorrow.
I don't live for tomorrow. Never saw the fun in it.
-- Denny Crane, Boston Legal


Attachments:
(No filename) (0.00 B)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-04 04:33:52

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Fri, Dec 31, 2010 at 09:51:04AM +0100, Mario 'BitKoenig' Holbe wrote:
> On Fri, Dec 31, 2010 at 01:46:53PM +1100, Herbert Xu wrote:
> > Thanks for checking. Can you provide the output of
> > cat /proc/cpuinfo
>
> attached.

Thanks.

Can you please test the following patch?

diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c
index 794aacb..507a57f 100644
--- a/drivers/char/hw_random/via-rng.c
+++ b/drivers/char/hw_random/via-rng.c
@@ -24,6 +24,7 @@
* warranty of any kind, whether express or implied.
*/

+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/hw_random.h>
@@ -34,7 +35,6 @@
#include <asm/i387.h>


-#define PFX KBUILD_MODNAME ": "


enum {
@@ -90,8 +90,10 @@ static inline u32 xstore(u32 *addr, u32 edx_in)

static int via_rng_data_present(struct hwrng *rng, int wait)
{
+ char buf[16 + PADLOCK_ALIGNMENT - STACK_ALIGN] __attribute__
+ ((aligned(STACK_ALIGN)));
+ u32 *via_rng_datum = (u32 *)PTR_ALIGN(&buf[0], PADLOCK_ALIGNMENT);
u32 bytes_out;
- u32 *via_rng_datum = (u32 *)(&rng->priv);
int i;

/* We choose the recommended 1-byte-per-instruction RNG rate,
@@ -115,6 +117,7 @@ static int via_rng_data_present(struct hwrng *rng, int wait)
break;
udelay(10);
}
+ rng->priv = *via_rng_datum;
return bytes_out ? 1 : 0;
}

diff --git a/drivers/crypto/padlock-aes.c b/drivers/crypto/padlock-aes.c
index 2e992bc..2e56508 100644
--- a/drivers/crypto/padlock-aes.c
+++ b/drivers/crypto/padlock-aes.c
@@ -9,6 +9,7 @@

#include <crypto/algapi.h>
#include <crypto/aes.h>
+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
@@ -21,7 +22,6 @@
#include <asm/byteorder.h>
#include <asm/processor.h>
#include <asm/i387.h>
-#include "padlock.h"

/*
* Number of data blocks actually fetched for each xcrypt insn.
diff --git a/drivers/crypto/padlock-sha.c b/drivers/crypto/padlock-sha.c
index d3a27e0..adf075b 100644
--- a/drivers/crypto/padlock-sha.c
+++ b/drivers/crypto/padlock-sha.c
@@ -13,6 +13,7 @@
*/

#include <crypto/internal/hash.h>
+#include <crypto/padlock.h>
#include <crypto/sha.h>
#include <linux/err.h>
#include <linux/module.h>
@@ -22,13 +23,6 @@
#include <linux/kernel.h>
#include <linux/scatterlist.h>
#include <asm/i387.h>
-#include "padlock.h"
-
-#ifdef CONFIG_64BIT
-#define STACK_ALIGN 16
-#else
-#define STACK_ALIGN 4
-#endif

struct padlock_sha_desc {
struct shash_desc fallback;
diff --git a/drivers/crypto/padlock.h b/drivers/crypto/padlock.h
deleted file mode 100644
index b728e45..0000000
--- a/drivers/crypto/padlock.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Driver for VIA PadLock
- *
- * Copyright (c) 2004 Michal Ludvig <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or (at your option)
- * any later version.
- *
- */
-
-#ifndef _CRYPTO_PADLOCK_H
-#define _CRYPTO_PADLOCK_H
-
-#define PADLOCK_ALIGNMENT 16
-
-#define PFX "padlock: "
-
-#define PADLOCK_CRA_PRIORITY 300
-#define PADLOCK_COMPOSITE_PRIORITY 400
-
-#endif /* _CRYPTO_PADLOCK_H */
diff --git a/include/crypto/padlock.h b/include/crypto/padlock.h
new file mode 100644
index 0000000..d2cfa2e
--- /dev/null
+++ b/include/crypto/padlock.h
@@ -0,0 +1,29 @@
+/*
+ * Driver for VIA PadLock
+ *
+ * Copyright (c) 2004 Michal Ludvig <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ */
+
+#ifndef _CRYPTO_PADLOCK_H
+#define _CRYPTO_PADLOCK_H
+
+#define PADLOCK_ALIGNMENT 16
+
+#define PFX KBUILD_MODNAME ": "
+
+#define PADLOCK_CRA_PRIORITY 300
+#define PADLOCK_COMPOSITE_PRIORITY 400
+
+#ifdef CONFIG_64BIT
+#define STACK_ALIGN 16
+#else
+#define STACK_ALIGN 4
+#endif
+
+#endif /* _CRYPTO_PADLOCK_H */

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2011-01-04 12:20:23

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

Hello Herbert,

On Tue, Jan 04, 2011 at 03:33:38PM +1100, Herbert Xu wrote:
> Can you please test the following patch?
> diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c

Hmmm, yes - the patch fixes the crashes, i.e. no more crashes with
either sequence of module-loading, cat rng_available works as well,
but...

Having this patch active rngd complains:
rngd[1435]: rngd 2-unofficial-mt.13 starting up...
rngd[1435]: block failed FIPS test: 0x1f
rngd[1435]: block failed FIPS test: 0x1f
...
rngd[1435]: stats: entropy added to kernel pool: 0
rngd[1435]: stats: FIPS 140-2 successes: 0
rngd[1435]: stats: FIPS 140-2 failures: 10

It doesn't do this without the patch.
The only available rng was via, I did blacklist the others just to be
sure.


regards
Mario
--
The social dynamics of the net are a direct consequence of the fact that
nobody has yet developed a Remote Strangulation Protocol. -- Larry Wall


Attachments:
(No filename) (952.00 B)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-04 12:38:35

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Tue, Jan 04, 2011 at 01:19:57PM +0100, Mario 'BitKoenig' Holbe wrote:
>
> Hmmm, yes - the patch fixes the crashes, i.e. no more crashes with
> either sequence of module-loading, cat rng_available works as well,
> but...
>
> Having this patch active rngd complains:
> rngd[1435]: rngd 2-unofficial-mt.13 starting up...
> rngd[1435]: block failed FIPS test: 0x1f
> rngd[1435]: block failed FIPS test: 0x1f
> ...
> rngd[1435]: stats: entropy added to kernel pool: 0
> rngd[1435]: stats: FIPS 140-2 successes: 0
> rngd[1435]: stats: FIPS 140-2 failures: 10
>
> It doesn't do this without the patch.
> The only available rng was via, I did blacklist the others just to be
> sure.

Hmm, can you print out what it's actually producing (e.g., by
stracing rngd)?

Can you also double-check that this doesn't happen with Larry's
patch?

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2011-01-04 12:57:43

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Tue, Jan 04, 2011 at 11:38:24PM +1100, Herbert Xu wrote:
> On Tue, Jan 04, 2011 at 01:19:57PM +0100, Mario 'BitKoenig' Holbe wrote:
> >
> > Hmmm, yes - the patch fixes the crashes, i.e. no more crashes with
> > either sequence of module-loading, cat rng_available works as well,
> > but...
> >
> > Having this patch active rngd complains:
> > rngd[1435]: rngd 2-unofficial-mt.13 starting up...
> > rngd[1435]: block failed FIPS test: 0x1f
> > rngd[1435]: block failed FIPS test: 0x1f
> > ...
> > rngd[1435]: stats: entropy added to kernel pool: 0
> > rngd[1435]: stats: FIPS 140-2 successes: 0
> > rngd[1435]: stats: FIPS 140-2 failures: 10
> >
> > It doesn't do this without the patch.
> > The only available rng was via, I did blacklist the others just to be
> > sure.
>
> Hmm, can you print out what it's actually producing (e.g., by
> stracing rngd)?

# ps -ef | grep 'rng[d]'
# cat /sys/devices/virtual/misc/hw_random/rng_available
via
# hexdump -n 512 -C /dev/hwrng
00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00000010 ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00 |................|
00000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
00000200
# hexdump -n 512 -C /dev/hwrng
00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00000070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
00000200
# hexdump -n 1024 -C /dev/hwrng
00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00000060 ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 |................|
00000070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
00000400
# hexdump -n 1024 -C /dev/hwrng
00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
*
00000070 ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
00000080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
00000400
#
> Can you also double-check that this doesn't happen with Larry's
> patch?

Nope, it doesn't do this with Larry's patch.


Mario
--
The Encyclopedia Galactica, in its chapter on Love states that it is far
too complicated to define.
The Hitchhiker's Guide to the Galaxy has this to say on the subject of
love: Avoid, if at all possible.


Attachments:
(No filename) (2.63 kB)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-04 22:42:48

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Tue, Jan 04, 2011 at 01:57:22PM +0100, Mario 'BitKoenig' Holbe wrote:
>
> # hexdump -n 512 -C /dev/hwrng
> 00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
> 00000010 ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00 |................|
> 00000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *

Weird.

Can you please try this patch against vanilla to print out the
raw output of xstore?

diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c
index 794aacb..4408d4e 100644
--- a/drivers/char/hw_random/via-rng.c
+++ b/drivers/char/hw_random/via-rng.c
@@ -24,6 +24,7 @@
* warranty of any kind, whether express or implied.
*/

+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/hw_random.h>
@@ -34,7 +35,6 @@
#include <asm/i387.h>


-#define PFX KBUILD_MODNAME ": "


enum {
@@ -85,13 +85,16 @@ static inline u32 xstore(u32 *addr, u32 edx_in)
:"D"(addr), "d"(edx_in));

irq_ts_restore(ts_state);
+ printk(KERN_DEBUG "0x%x\n", *addr);
return eax_out;
}

static int via_rng_data_present(struct hwrng *rng, int wait)
{
+ char buf[16 + PADLOCK_ALIGNMENT - STACK_ALIGN] __attribute__
+ ((aligned(STACK_ALIGN)));
+ u32 *via_rng_datum = (u32 *)PTR_ALIGN(&buf[0], PADLOCK_ALIGNMENT);
u32 bytes_out;
- u32 *via_rng_datum = (u32 *)(&rng->priv);
int i;

/* We choose the recommended 1-byte-per-instruction RNG rate,
@@ -115,6 +118,7 @@ static int via_rng_data_present(struct hwrng *rng, int wait)
break;
udelay(10);
}
+ rng->priv = *via_rng_datum;
return bytes_out ? 1 : 0;
}

diff --git a/drivers/crypto/padlock-aes.c b/drivers/crypto/padlock-aes.c
index 2e992bc..2e56508 100644
--- a/drivers/crypto/padlock-aes.c
+++ b/drivers/crypto/padlock-aes.c
@@ -9,6 +9,7 @@

#include <crypto/algapi.h>
#include <crypto/aes.h>
+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
@@ -21,7 +22,6 @@
#include <asm/byteorder.h>
#include <asm/processor.h>
#include <asm/i387.h>
-#include "padlock.h"

/*
* Number of data blocks actually fetched for each xcrypt insn.
diff --git a/drivers/crypto/padlock-sha.c b/drivers/crypto/padlock-sha.c
index d3a27e0..adf075b 100644
--- a/drivers/crypto/padlock-sha.c
+++ b/drivers/crypto/padlock-sha.c
@@ -13,6 +13,7 @@
*/

#include <crypto/internal/hash.h>
+#include <crypto/padlock.h>
#include <crypto/sha.h>
#include <linux/err.h>
#include <linux/module.h>
@@ -22,13 +23,6 @@
#include <linux/kernel.h>
#include <linux/scatterlist.h>
#include <asm/i387.h>
-#include "padlock.h"
-
-#ifdef CONFIG_64BIT
-#define STACK_ALIGN 16
-#else
-#define STACK_ALIGN 4
-#endif

struct padlock_sha_desc {
struct shash_desc fallback;
diff --git a/drivers/crypto/padlock.h b/drivers/crypto/padlock.h
deleted file mode 100644
index b728e45..0000000
--- a/drivers/crypto/padlock.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Driver for VIA PadLock
- *
- * Copyright (c) 2004 Michal Ludvig <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or (at your option)
- * any later version.
- *
- */
-
-#ifndef _CRYPTO_PADLOCK_H
-#define _CRYPTO_PADLOCK_H
-
-#define PADLOCK_ALIGNMENT 16
-
-#define PFX "padlock: "
-
-#define PADLOCK_CRA_PRIORITY 300
-#define PADLOCK_COMPOSITE_PRIORITY 400
-
-#endif /* _CRYPTO_PADLOCK_H */
diff --git a/include/crypto/padlock.h b/include/crypto/padlock.h
new file mode 100644
index 0000000..d2cfa2e
--- /dev/null
+++ b/include/crypto/padlock.h
@@ -0,0 +1,29 @@
+/*
+ * Driver for VIA PadLock
+ *
+ * Copyright (c) 2004 Michal Ludvig <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ */
+
+#ifndef _CRYPTO_PADLOCK_H
+#define _CRYPTO_PADLOCK_H
+
+#define PADLOCK_ALIGNMENT 16
+
+#define PFX KBUILD_MODNAME ": "
+
+#define PADLOCK_CRA_PRIORITY 300
+#define PADLOCK_COMPOSITE_PRIORITY 400
+
+#ifdef CONFIG_64BIT
+#define STACK_ALIGN 16
+#else
+#define STACK_ALIGN 4
+#endif
+
+#endif /* _CRYPTO_PADLOCK_H */

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2011-01-04 23:07:01

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 09:42:38AM +1100, Herbert Xu wrote:
> Can you please try this patch against vanilla to print out the
> raw output of xstore?

# ps -ef | grep 'rng[d]'
# cat /sys/devices/virtual/misc/hw_random/rng_available
via
# hexdump -n 16 -C /dev/hwrng
00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00000010
#

kern.log meanwhile (lines numbered):
1 Jan 4 23:56:40 ideapad kernel: [ 151.714225] 0x0
2 Jan 4 23:56:40 ideapad kernel: [ 151.714236] 0xffffffff
3 Jan 4 23:56:40 ideapad kernel: [ 151.714239] 0x0
4 Jan 4 23:56:40 ideapad kernel: [ 151.714251] 0xffffffff
...
233 Jan 4 23:56:40 ideapad kernel: [ 151.715967] 0x0
234 Jan 4 23:56:40 ideapad kernel: [ 151.715980] 0xffffffff
235 Jan 4 23:56:40 ideapad kernel: [ 151.715982] 0x0
236 Jan 4 23:56:40 ideapad kernel: [ 151.715995] 0xffffffff
237 Jan 4 23:56:40 ideapad kernel: [ 151.720342] 0x0
238 Jan 4 23:56:40 ideapad kernel: [ 151.720347] 0x0
239 Jan 4 23:56:40 ideapad kernel: [ 151.720361] 0x0
240 Jan 4 23:56:40 ideapad kernel: [ 151.720365] 0x0
...
8163 Jan 4 23:56:40 ideapad kernel: [ 151.987049] 0x0
8164 Jan 4 23:56:40 ideapad kernel: [ 151.987061] 0x0
8165 Jan 4 23:56:40 ideapad kernel: [ 151.987063] 0x0
8166 Jan 4 23:56:40 ideapad kernel: [ 151.987075] 0x0


Mario
--
If her DNA was off by one percentage point, she'd be a dolphin.
-- Dr. Gregory House


Attachments:
(No filename) (1.47 kB)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-04 23:26:52

by Larry Finger

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On 01/04/2011 05:06 PM, Mario 'BitKoenig' Holbe wrote:
> On Wed, Jan 05, 2011 at 09:42:38AM +1100, Herbert Xu wrote:
>> Can you please try this patch against vanilla to print out the
>> raw output of xstore?
>
> # ps -ef | grep 'rng[d]'
> # cat /sys/devices/virtual/misc/hw_random/rng_available
> via
> # hexdump -n 16 -C /dev/hwrng
> 00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|

This thread just keeps getting more and more interesting! :)

For reference, this is what I get for b43:

hexdump -n 16 -C /dev/hwrng
00000000 c7 87 ec 43 fa ac a8 c2 3b 0d a6 8f 3e 35 54 ee |...C....;...>5T.|

Larry

2011-01-04 23:35:18

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 12:06:44AM +0100, Mario 'BitKoenig' Holbe wrote:
> On Wed, Jan 05, 2011 at 09:42:38AM +1100, Herbert Xu wrote:
> > Can you please try this patch against vanilla to print out the
> > raw output of xstore?
> 00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|

Btw: the same with vanilla 2.6.37-rc7 and only the printk in xstore()
from your 2nd patch:

# hexdump -n 16 -C /dev/hwrng
00000000 2b f3 16 07 e8 57 1a a9 d2 3e 7f ad 0e 78 75 7f |+....W...>...xu.|
00000010
#


kern.log meanwhile (lines numbered):

1 Jan 5 00:27:09 ideapad kernel: [ 122.489475] 0x25ba3b2b
2 Jan 5 00:27:09 ideapad kernel: [ 122.489486] 0xe68ae2f3
3 Jan 5 00:27:09 ideapad kernel: [ 122.489489] 0x0
4 Jan 5 00:27:09 ideapad kernel: [ 122.489502] 0x75621016
5 Jan 5 00:27:09 ideapad kernel: [ 122.489505] 0x0
6 Jan 5 00:27:09 ideapad kernel: [ 122.489518] 0x727edc07
7 Jan 5 00:27:09 ideapad kernel: [ 122.489520] 0x0
8 Jan 5 00:27:09 ideapad kernel: [ 122.489534] 0x851b82e8
9 Jan 5 00:27:09 ideapad kernel: [ 122.491241] 0xfba49c57
10 Jan 5 00:27:09 ideapad kernel: [ 122.491245] 0x0
11 Jan 5 00:27:09 ideapad kernel: [ 122.491258] 0xc63eca1a
...
8156 Jan 5 00:27:10 ideapad kernel: [ 122.782215] 0x0
8157 Jan 5 00:27:10 ideapad kernel: [ 122.782228] 0xb736d99b
8158 Jan 5 00:27:10 ideapad kernel: [ 122.782230] 0x0
8159 Jan 5 00:27:10 ideapad kernel: [ 122.782243] 0x339211fd
8160 Jan 5 00:27:10 ideapad kernel: [ 122.782246] 0x0
8161 Jan 5 00:27:10 ideapad kernel: [ 122.782258] 0x12f898d1
8162 Jan 5 00:27:10 ideapad kernel: [ 122.782261] 0x0
8163 Jan 5 00:27:10 ideapad kernel: [ 122.782273] 0xc73fcdda


Mario
--
The question of whether a computer can think is no more interesting than
the question of whether a submarine can swim. -- E. W. Dijkstra


Attachments:
(No filename) (1.88 kB)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-05 00:14:09

by Larry Finger

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On 01/04/2011 04:42 PM, Herbert Xu wrote:
> On Tue, Jan 04, 2011 at 01:57:22PM +0100, Mario 'BitKoenig' Holbe wrote:
>>
>> # hexdump -n 512 -C /dev/hwrng
>> 00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>> 00000010 ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00 |................|
>> 00000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> *
>
> Weird.
>
> Can you please try this patch against vanilla to print out the
> raw output of xstore?
>
> diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c
> index 794aacb..4408d4e 100644
> --- a/drivers/char/hw_random/via-rng.c
> +++ b/drivers/char/hw_random/via-rng.c
> @@ -24,6 +24,7 @@
> * warranty of any kind, whether express or implied.
> */
>
> +#include <crypto/padlock.h>
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/hw_random.h>
> @@ -34,7 +35,6 @@
> #include <asm/i387.h>
>
>
> -#define PFX KBUILD_MODNAME ": "
>
>
> enum {
> @@ -85,13 +85,16 @@ static inline u32 xstore(u32 *addr, u32 edx_in)
> :"D"(addr), "d"(edx_in));
>
> irq_ts_restore(ts_state);
> + printk(KERN_DEBUG "0x%x\n", *addr);
> return eax_out;
> }
>
> static int via_rng_data_present(struct hwrng *rng, int wait)
> {
> + char buf[16 + PADLOCK_ALIGNMENT - STACK_ALIGN] __attribute__
> + ((aligned(STACK_ALIGN)));
> + u32 *via_rng_datum = (u32 *)PTR_ALIGN(&buf[0], PADLOCK_ALIGNMENT);

If I didn't get lost in expanding all those macros, I think the above can end up
with what is essentially a negative value for the index of buf. Shouldn't the
right-hand side of the statement be

(u32 *)PTR_ALIGN(&buf[PADLOCK_ALIGNMENT], PADLOCK_ALIGNMENT);

That resolves to an index for buf from 0 to (PADLOCK_ALIGNMENT - 1).

Larry

2011-01-05 00:19:47

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Tue, Jan 04, 2011 at 06:14:16PM -0600, Larry Finger wrote:
>
> If I didn't get lost in expanding all those macros, I think the above can end up
> with what is essentially a negative value for the index of buf. Shouldn't the
> right-hand side of the statement be
>
> (u32 *)PTR_ALIGN(&buf[PADLOCK_ALIGNMENT], PADLOCK_ALIGNMENT);
>
> That resolves to an index for buf from 0 to (PADLOCK_ALIGNMENT - 1).

PTR_ALIGN (and ALIGN) rounds up, not down.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2011-01-05 00:30:26

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 12:06:44AM +0100, Mario 'BitKoenig' Holbe wrote:
>
> kern.log meanwhile (lines numbered):
> 1 Jan 4 23:56:40 ideapad kernel: [ 151.714225] 0x0
> 2 Jan 4 23:56:40 ideapad kernel: [ 151.714236] 0xffffffff
> 3 Jan 4 23:56:40 ideapad kernel: [ 151.714239] 0x0
> 4 Jan 4 23:56:40 ideapad kernel: [ 151.714251] 0xffffffff

OK, so xstore really is producing crap. Can you try this path
(also against vanilla) to print out some extra info?

diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c
index 794aacb..d7ef7ac 100644
--- a/drivers/char/hw_random/via-rng.c
+++ b/drivers/char/hw_random/via-rng.c
@@ -24,6 +24,7 @@
* warranty of any kind, whether express or implied.
*/

+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/hw_random.h>
@@ -34,7 +35,6 @@
#include <asm/i387.h>


-#define PFX KBUILD_MODNAME ": "


enum {
@@ -85,13 +85,16 @@ static inline u32 xstore(u32 *addr, u32 edx_in)
:"D"(addr), "d"(edx_in));

irq_ts_restore(ts_state);
+ printk(KERN_DEBUG "%p 0x%x 0x%x\n", addr, *addr, eax_out);
return eax_out;
}

static int via_rng_data_present(struct hwrng *rng, int wait)
{
+ char buf[16 + PADLOCK_ALIGNMENT - STACK_ALIGN] __attribute__
+ ((aligned(STACK_ALIGN)));
+ u32 *via_rng_datum = (u32 *)PTR_ALIGN(&buf[0], PADLOCK_ALIGNMENT);
u32 bytes_out;
- u32 *via_rng_datum = (u32 *)(&rng->priv);
int i;

/* We choose the recommended 1-byte-per-instruction RNG rate,
@@ -115,6 +118,7 @@ static int via_rng_data_present(struct hwrng *rng, int wait)
break;
udelay(10);
}
+ rng->priv = *via_rng_datum;
return bytes_out ? 1 : 0;
}

diff --git a/drivers/crypto/padlock-aes.c b/drivers/crypto/padlock-aes.c
index 2e992bc..2e56508 100644
--- a/drivers/crypto/padlock-aes.c
+++ b/drivers/crypto/padlock-aes.c
@@ -9,6 +9,7 @@

#include <crypto/algapi.h>
#include <crypto/aes.h>
+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
@@ -21,7 +22,6 @@
#include <asm/byteorder.h>
#include <asm/processor.h>
#include <asm/i387.h>
-#include "padlock.h"

/*
* Number of data blocks actually fetched for each xcrypt insn.
diff --git a/drivers/crypto/padlock-sha.c b/drivers/crypto/padlock-sha.c
index d3a27e0..adf075b 100644
--- a/drivers/crypto/padlock-sha.c
+++ b/drivers/crypto/padlock-sha.c
@@ -13,6 +13,7 @@
*/

#include <crypto/internal/hash.h>
+#include <crypto/padlock.h>
#include <crypto/sha.h>
#include <linux/err.h>
#include <linux/module.h>
@@ -22,13 +23,6 @@
#include <linux/kernel.h>
#include <linux/scatterlist.h>
#include <asm/i387.h>
-#include "padlock.h"
-
-#ifdef CONFIG_64BIT
-#define STACK_ALIGN 16
-#else
-#define STACK_ALIGN 4
-#endif

struct padlock_sha_desc {
struct shash_desc fallback;
diff --git a/drivers/crypto/padlock.h b/drivers/crypto/padlock.h
deleted file mode 100644
index b728e45..0000000
--- a/drivers/crypto/padlock.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Driver for VIA PadLock
- *
- * Copyright (c) 2004 Michal Ludvig <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or (at your option)
- * any later version.
- *
- */
-
-#ifndef _CRYPTO_PADLOCK_H
-#define _CRYPTO_PADLOCK_H
-
-#define PADLOCK_ALIGNMENT 16
-
-#define PFX "padlock: "
-
-#define PADLOCK_CRA_PRIORITY 300
-#define PADLOCK_COMPOSITE_PRIORITY 400
-
-#endif /* _CRYPTO_PADLOCK_H */
diff --git a/include/crypto/padlock.h b/include/crypto/padlock.h
new file mode 100644
index 0000000..d2cfa2e
--- /dev/null
+++ b/include/crypto/padlock.h
@@ -0,0 +1,29 @@
+/*
+ * Driver for VIA PadLock
+ *
+ * Copyright (c) 2004 Michal Ludvig <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ */
+
+#ifndef _CRYPTO_PADLOCK_H
+#define _CRYPTO_PADLOCK_H
+
+#define PADLOCK_ALIGNMENT 16
+
+#define PFX KBUILD_MODNAME ": "
+
+#define PADLOCK_CRA_PRIORITY 300
+#define PADLOCK_COMPOSITE_PRIORITY 400
+
+#ifdef CONFIG_64BIT
+#define STACK_ALIGN 16
+#else
+#define STACK_ALIGN 4
+#endif
+
+#endif /* _CRYPTO_PADLOCK_H */

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2011-01-05 01:38:44

by Larry Finger

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On 01/04/2011 06:19 PM, Herbert Xu wrote:
>
> PTR_ALIGN (and ALIGN) rounds up, not down.

I see where I got lost in the macro expansions.

I'm still trying to understand the need for the __attribute__ on the definition
for buf.

Larry

2011-01-05 01:45:59

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 11:30:20AM +1100, Herbert Xu wrote:
> OK, so xstore really is producing crap. Can you try this path
> (also against vanilla) to print out some extra info?
>
> irq_ts_restore(ts_state);
> + printk(KERN_DEBUG "%p 0x%x 0x%x\n", addr, *addr, eax_out);
> return eax_out;

# dd if=/dev/hwrng bs=16 count=1 | hexdump -C
1+0 records in
1+0 records out
16 bytes (16 B) copied, 0,000288649 s, 55,4 kB/s
00000000 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00000010
#

Jan 5 02:25:00 ideapad kernel: [ 112.073759] f3cc3f38 0x0 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073770] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073774] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073788] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073791] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073805] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073808] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073821] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073825] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073838] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073841] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073855] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073858] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073872] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073875] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073888] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073892] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073905] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073908] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073922] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073925] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073938] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073942] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073955] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073959] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073972] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073975] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.073989] f3cc3f38 0xffffffff 0x8
Jan 5 02:25:00 ideapad kernel: [ 112.073992] f3cc3f30 0x0 0x0
Jan 5 02:25:00 ideapad kernel: [ 112.074005] f3cc3f38 0xffffffff 0x8


Mario
--
Oh Du mein Koenig ... Eine Netzgroesse schrieb mal sinngemaess:
Du musst es so lesen wie ich es meine, nicht so wie ich es schreibe.
Ich meine es natuerlich so, wie Du es schreibst 8--)
-- O.G. Schwenk in de.comm.chatsystems


Attachments:
(No filename) (2.82 kB)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-05 03:52:36

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 11:30:20AM +1100, Herbert Xu wrote:
> OK, so xstore really is producing crap. Can you try this path
> (also against vanilla) to print out some extra info?

All right, here is what happens...

On top of your 3rd patch:

--- linux-source-2.6.37-rc7/drivers/char/hw_random/via-rng.c.hxu3 2011-01-05 04:13:09.452322117 +0100
+++ linux-source-2.6.37-rc7/drivers/char/hw_random/via-rng.c 2011-01-05 03:59:33.169316276 +0100
@@ -97,6 +97,10 @@ static int via_rng_data_present(struct h
u32 bytes_out;
int i;

+ printk(KERN_DEBUG "buf=%p + %zu, via_rng_datum=%p\n",
+ buf, sizeof(buf), via_rng_datum);
+
+
/* We choose the recommended 1-byte-per-instruction RNG rate,
* for greater randomness at the expense of speed. Larger
* values 2, 4, or 8 bytes-per-instruction yield greater

gives:

[ 103.276337] buf=f6e23f28 + 28, via_rng_datum=f6e23f30
[ 103.276351] f6e23f38 0x0 0x8
[ 103.276371] buf=f6e23f28 + 28, via_rng_datum=f6e23f30
[ 103.276380] f6e23f38 0xffffffff 0x8
...

According to the VIA PadLock Programming Guide, XSTORE increments EDI by
the number of bytes stored.
This did obviously not matter as long as the buffer was "sufficiently
distant", but now you have the buffer close on the stack and I believe,
the optimizer crops up, hence the EDI increment begins to matter.

IMHO EDI (and EDX - for completeness :)) should be put on the asm
clobber-list, but if I try to do it, I always get:
error: can't find a register in class 'DIREG' while reloading 'asm'
error: 'asm' operand has impossible constraints

So... I have the reason - solution is up to you :)

Btw: the 8 bytes increment (as well as the 8 in EAX 4:0) proves that
XSTOR indeed writes more than 32bit :)


g'nite
Mario
--
The only thing to be scared of, son, is tomorrow.
I don't live for tomorrow. Never saw the fun in it.
-- Denny Crane, Boston Legal


Attachments:
(No filename) (1.85 kB)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-05 05:47:48

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 04:52:22AM +0100, Mario 'BitKoenig' Holbe wrote:
>
> According to the VIA PadLock Programming Guide, XSTORE increments EDI by
> the number of bytes stored.
> This did obviously not matter as long as the buffer was "sufficiently
> distant", but now you have the buffer close on the stack and I believe,
> the optimizer crops up, hence the EDI increment begins to matter.

Interesting. So your compiler was producing incorrect output.
What version of gcc were you using?

Please attach the assembly output of the function in question.

Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2011-01-05 13:16:22

by Mario 'BitKoenig' Holbe

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 04:47:35PM +1100, Herbert Xu wrote:
> On Wed, Jan 05, 2011 at 04:52:22AM +0100, Mario 'BitKoenig' Holbe wrote:
> > According to the VIA PadLock Programming Guide, XSTORE increments EDI by
> > the number of bytes stored.
> > This did obviously not matter as long as the buffer was "sufficiently
> > distant", but now you have the buffer close on the stack and I believe,
> > the optimizer crops up, hence the EDI increment begins to matter.
> Interesting. So your compiler was producing incorrect output.

Why incorrect? How should the compiler know that EDI gets modified?
It's listed as XSTORE input only.

> What version of gcc were you using?

gcc version 4.4.5 (Debian 4.4.5-10)

> Please attach the assembly output of the function in question.

attached. I hope I got the gcc call right. However, I prefer the objdump
output anyways, so this one is attached as well.

As you can see (from both, maybe less from the one, more from the
other), it is as I supposed it to be: the optimizer uses EDI to store
via_rng_datum - reasonable, since EDI is a required input for the asm()
directive anyways. And since it doesn't know EDI gets modified, it just
continues using it as via_rng_datum afterwards.

Btw: this is also the reason why it did work before: before, &rng->priv
was never used again in a close-enough (and static enough) context, so
it didn't matter whether EDI (which surely was used as &rng->priv) was
clobbered or not.

The IMHO best solution would be to tell the compiler that EDI gets
clobbered: attached via-rng1-preferred.patch to apply on top of your
first patch. However, as I already said, the compiler starts whining
with either of both inputs on the clobber-list (don't really know why it
cries for edx, but well...).

The overkill solution is to preserve EDI manually: attached
via-rng1-overkill.patch to apply on top of your first patch.
And... yes, this works, really :)

As I already said in my mail before: IMHO, for completeness, EDX should
be preserved as well: the PadLock Quick Reference states the upper 30
bits of EDX will be zeroed by XSTORE, the VIA PadLock Programming Guide
states they may be zeroed.
This does currently not really matter, since a) VIA_RNG_CHUNK_1 (0x03)
is quite zero in the upper 30 bits, and b) the XSTORE quality factor is
only defined with 2 bits.
Though it's hard to believe this could ever change, I could imagine
future code that, for example, tries to balance quality/speed, and
chooses different values for EDX (and overloads the upper 30 bits for
some additional internal information) and after xstore() behaves
different based on the value chosen before. Then, it would matter.


Mario
--
It is practically impossible to teach good programming style to students
that have had prior exposure to BASIC: as potential programmers they are
mentally mutilated beyond hope of regeneration. -- Dijkstra


Attachments:
(No filename) (0.00 B)
signature.asc (482.00 B)
Digital signature
Download all attachments

2011-01-06 06:12:59

by Herbert Xu

[permalink] [raw]
Subject: Re: 2.6.37-rc7: Regression: b43: crashes in hwrng_register()

On Wed, Jan 05, 2011 at 02:16:22PM +0100, Mario 'BitKoenig' Holbe wrote:
>
> attached. I hope I got the gcc call right. However, I prefer the objdump
> output anyways, so this one is attached as well.

I see what you mean now.

Please let me know if this patch (still against vanilla) helps.

diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c
index 794aacb..6788bbc 100644
--- a/drivers/char/hw_random/via-rng.c
+++ b/drivers/char/hw_random/via-rng.c
@@ -24,6 +24,7 @@
* warranty of any kind, whether express or implied.
*/

+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/hw_random.h>
@@ -34,7 +35,6 @@
#include <asm/i387.h>


-#define PFX KBUILD_MODNAME ": "


enum {
@@ -81,8 +81,13 @@ static inline u32 xstore(u32 *addr, u32 edx_in)
ts_state = irq_ts_save();

asm(".byte 0x0F,0xA7,0xC0 /* xstore %%edi (addr=%0) */"
- :"=m"(*addr), "=a"(eax_out)
- :"D"(addr), "d"(edx_in));
+ : "=m" (*addr), "=a" (eax_out), "+D" (addr)
+ : "d" (edx_in)
+#ifdef CONFIG_64BIT
+ : "%rcx");
+#else
+ : "%ecx");
+#endif

irq_ts_restore(ts_state);
return eax_out;
@@ -90,8 +95,10 @@ static inline u32 xstore(u32 *addr, u32 edx_in)

static int via_rng_data_present(struct hwrng *rng, int wait)
{
+ char buf[16 + PADLOCK_ALIGNMENT - STACK_ALIGN] __attribute__
+ ((aligned(STACK_ALIGN)));
+ u32 *via_rng_datum = (u32 *)PTR_ALIGN(&buf[0], PADLOCK_ALIGNMENT);
u32 bytes_out;
- u32 *via_rng_datum = (u32 *)(&rng->priv);
int i;

/* We choose the recommended 1-byte-per-instruction RNG rate,
@@ -115,6 +122,7 @@ static int via_rng_data_present(struct hwrng *rng, int wait)
break;
udelay(10);
}
+ rng->priv = *via_rng_datum;
return bytes_out ? 1 : 0;
}

diff --git a/drivers/crypto/padlock-aes.c b/drivers/crypto/padlock-aes.c
index 2e992bc..2e56508 100644
--- a/drivers/crypto/padlock-aes.c
+++ b/drivers/crypto/padlock-aes.c
@@ -9,6 +9,7 @@

#include <crypto/algapi.h>
#include <crypto/aes.h>
+#include <crypto/padlock.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
@@ -21,7 +22,6 @@
#include <asm/byteorder.h>
#include <asm/processor.h>
#include <asm/i387.h>
-#include "padlock.h"

/*
* Number of data blocks actually fetched for each xcrypt insn.
diff --git a/drivers/crypto/padlock-sha.c b/drivers/crypto/padlock-sha.c
index d3a27e0..adf075b 100644
--- a/drivers/crypto/padlock-sha.c
+++ b/drivers/crypto/padlock-sha.c
@@ -13,6 +13,7 @@
*/

#include <crypto/internal/hash.h>
+#include <crypto/padlock.h>
#include <crypto/sha.h>
#include <linux/err.h>
#include <linux/module.h>
@@ -22,13 +23,6 @@
#include <linux/kernel.h>
#include <linux/scatterlist.h>
#include <asm/i387.h>
-#include "padlock.h"
-
-#ifdef CONFIG_64BIT
-#define STACK_ALIGN 16
-#else
-#define STACK_ALIGN 4
-#endif

struct padlock_sha_desc {
struct shash_desc fallback;
diff --git a/drivers/crypto/padlock.h b/drivers/crypto/padlock.h
deleted file mode 100644
index b728e45..0000000
--- a/drivers/crypto/padlock.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Driver for VIA PadLock
- *
- * Copyright (c) 2004 Michal Ludvig <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or (at your option)
- * any later version.
- *
- */
-
-#ifndef _CRYPTO_PADLOCK_H
-#define _CRYPTO_PADLOCK_H
-
-#define PADLOCK_ALIGNMENT 16
-
-#define PFX "padlock: "
-
-#define PADLOCK_CRA_PRIORITY 300
-#define PADLOCK_COMPOSITE_PRIORITY 400
-
-#endif /* _CRYPTO_PADLOCK_H */
diff --git a/include/crypto/padlock.h b/include/crypto/padlock.h
new file mode 100644
index 0000000..d2cfa2e
--- /dev/null
+++ b/include/crypto/padlock.h
@@ -0,0 +1,29 @@
+/*
+ * Driver for VIA PadLock
+ *
+ * Copyright (c) 2004 Michal Ludvig <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ */
+
+#ifndef _CRYPTO_PADLOCK_H
+#define _CRYPTO_PADLOCK_H
+
+#define PADLOCK_ALIGNMENT 16
+
+#define PFX KBUILD_MODNAME ": "
+
+#define PADLOCK_CRA_PRIORITY 300
+#define PADLOCK_COMPOSITE_PRIORITY 400
+
+#ifdef CONFIG_64BIT
+#define STACK_ALIGN 16
+#else
+#define STACK_ALIGN 4
+#endif
+
+#endif /* _CRYPTO_PADLOCK_H */

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt