[PATCH] crypto: atmel-sha204a - drop hwrng quality reduction for ATSHA204A

[PATCH] crypto: atmel-sha204a - drop hwrng quality reduction for ATSHA204A

[Thorsten Blum]

Commit 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to
lowest possible") reduced the hwrng quality to 1 based on a review by
Bill Cox [1]. However, despite its title, the review only tested the
ATSHA204, not the ATSHA204A.

In the same thread, Atmel engineer Landon Cox wrote "this behavior has
been eliminated entirely"[2] in the ATSHA204A and "this problem does not
affect the ATECC108 or the ATECC108A (or the ATSHA204A)"[3].

According to the official ATSHA204A datasheet [4], the device contains a
high-quality hardware RNG that combines its output with an internal seed
value stored in EEPROM or SRAM to generate random numbers. The device
also implements all security functions using SHA-256, and the driver
uses the chip's Random command in seed-update mode.

Keep 'quality = 1' for ATSHA204, but drop the explicit hwrng quality
reduction for ATSHA204A and fall back to the hwrng core default.

[1] https://www.metzdowd.com/pipermail/cryptography/2014-December/023858.html
[2] https://www.metzdowd.com/pipermail/cryptography/2014-December/023852.html
[3] https://www.metzdowd.com/pipermail/cryptography/2014-December/023886.html
[4] https://ww1.microchip.com/downloads/en/DeviceDoc/ATSHA204A-Data-Sheet-40002025A.pdf

Fixes: 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to lowest possible")
Cc: stable@vger.kernel.org
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/crypto/atmel-sha204a.c | 40 ++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/atmel-sha204a.c b/drivers/crypto/atmel-sha204a.c
index dbb39ed0cea1..df69fb190e52 100644
--- a/drivers/crypto/atmel-sha204a.c
+++ b/drivers/crypto/atmel-sha204a.c
@@ -19,6 +19,25 @@
 #include <linux/workqueue.h>
 #include "atmel-i2c.h"
 
+enum atmel_sha204a_variant {
+	ATSHA204 = 1,
+	ATSHA204A,
+};
+
+static const struct of_device_id atmel_sha204a_dt_ids[] __maybe_unused = {
+	{ .compatible = "atmel,atsha204",  .data = (void *)ATSHA204 },
+	{ .compatible = "atmel,atsha204a", .data = (void *)ATSHA204A },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, atmel_sha204a_dt_ids);
+
+static const struct i2c_device_id atmel_sha204a_id[] = {
+	{ .name = "atsha204",  .driver_data = ATSHA204 },
+	{ .name = "atsha204a", .driver_data = ATSHA204A },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, atmel_sha204a_id);
+
 static void atmel_sha204a_rng_done(struct atmel_i2c_work_data *work_data,
 				   void *areq, int status)
 {
@@ -171,11 +190,8 @@ static int atmel_sha204a_probe(struct i2c_client *client)
 	i2c_priv->hwrng.name = dev_name(&client->dev);
 	i2c_priv->hwrng.read = atmel_sha204a_rng_read;
 
-	/*
-	 * According to review by Bill Cox [1], this HWRNG has very low entropy.
-	 * [1] https://www.metzdowd.com/pipermail/cryptography/2014-December/023858.html
-	 */
-	i2c_priv->hwrng.quality = 1;
+	if ((uintptr_t)i2c_get_match_data(client) == ATSHA204)
+		i2c_priv->hwrng.quality = 1;
 
 	ret = devm_hwrng_register(&client->dev, &i2c_priv->hwrng);
 	if (ret)
@@ -202,20 +218,6 @@ static void atmel_sha204a_remove(struct i2c_client *client)
 	kfree((void *)i2c_priv->hwrng.priv);
 }
 
-static const struct of_device_id atmel_sha204a_dt_ids[] __maybe_unused = {
-	{ .compatible = "atmel,atsha204", },
-	{ .compatible = "atmel,atsha204a", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, atmel_sha204a_dt_ids);
-
-static const struct i2c_device_id atmel_sha204a_id[] = {
-	{ "atsha204" },
-	{ "atsha204a" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(i2c, atmel_sha204a_id);
-
 static struct i2c_driver atmel_sha204a_driver = {
 	.probe			= atmel_sha204a_probe,
 	.remove			= atmel_sha204a_remove,

Re: [PATCH] crypto: atmel-sha204a - drop hwrng quality reduction for ATSHA204A

[Ard Biesheuvel]

Hi Thorsten,

On Mon, 27 Apr 2026, at 14:40, Thorsten Blum wrote:
> Commit 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to
> lowest possible") reduced the hwrng quality to 1 based on a review by
> Bill Cox [1]. However, despite its title, the review only tested the
> ATSHA204, not the ATSHA204A.
>
> In the same thread, Atmel engineer Landon Cox wrote "this behavior has
> been eliminated entirely"[2] in the ATSHA204A and "this problem does not
> affect the ATECC108 or the ATECC108A (or the ATSHA204A)"[3].
>
> According to the official ATSHA204A datasheet [4], the device contains a
> high-quality hardware RNG that combines its output with an internal seed
> value stored in EEPROM or SRAM to generate random numbers. The device
> also implements all security functions using SHA-256, and the driver
> uses the chip's Random command in seed-update mode.
>
> Keep 'quality = 1' for ATSHA204, but drop the explicit hwrng quality
> reduction for ATSHA204A and fall back to the hwrng core default.
>

Interesting! Thanks for digging this up.

> [1] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023858.html
> [2] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023852.html
> [3] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023886.html
> [4] 
> https://ww1.microchip.com/downloads/en/DeviceDoc/ATSHA204A-Data-Sheet-40002025A.pdf
>
> Fixes: 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to 
> lowest possible")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/crypto/atmel-sha204a.c | 40 ++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/crypto/atmel-sha204a.c b/drivers/crypto/atmel-sha204a.c
> index dbb39ed0cea1..df69fb190e52 100644
> --- a/drivers/crypto/atmel-sha204a.c
> +++ b/drivers/crypto/atmel-sha204a.c
> @@ -19,6 +19,25 @@
>  #include <linux/workqueue.h>
>  #include "atmel-i2c.h"
> 
> +enum atmel_sha204a_variant {
> +	ATSHA204 = 1,
> +	ATSHA204A,
> +};
> +

I agree that setting quality to '1' is only appropriate for the ATSHA204
but this looks a bit clunky to me.

Can we retain the comment here that you deleted below, and add
something like

static const unsigned short atsha204_quality = 1;

> +static const struct of_device_id atmel_sha204a_dt_ids[] __maybe_unused = {
> +	{ .compatible = "atmel,atsha204",  .data = (void *)ATSHA204 },
> +	{ .compatible = "atmel,atsha204a", .data = (void *)ATSHA204A },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, atmel_sha204a_dt_ids);
> +
> +static const struct i2c_device_id atmel_sha204a_id[] = {
> +	{ .name = "atsha204",  .driver_data = ATSHA204 },
> +	{ .name = "atsha204a", .driver_data = ATSHA204A },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, atmel_sha204a_id);
> +

Then, move these back to the old location, and point .data /
.driver_data to &atsha204_quality for atsha204 only.


>  static void atmel_sha204a_rng_done(struct atmel_i2c_work_data *work_data,
>  				   void *areq, int status)
>  {
> @@ -171,11 +190,8 @@ static int atmel_sha204a_probe(struct i2c_client *client)
>  	i2c_priv->hwrng.name = dev_name(&client->dev);
>  	i2c_priv->hwrng.read = atmel_sha204a_rng_read;
> 
> -	/*
> -	 * According to review by Bill Cox [1], this HWRNG has very low 
> entropy.
> -	 * [1] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023858.html
> -	 */
> -	i2c_priv->hwrng.quality = 1;
> +	if ((uintptr_t)i2c_get_match_data(client) == ATSHA204)
> +		i2c_priv->hwrng.quality = 1;
> 

Here you can override the field by dereferencing the match data if it
is non-NULL.

Alternatively, you could store the quality in the device_id structs
directly, but I think this is slightly more idiomatic.


>  	ret = devm_hwrng_register(&client->dev, &i2c_priv->hwrng);
>  	if (ret)
> @@ -202,20 +218,6 @@ static void atmel_sha204a_remove(struct i2c_client *client)
>  	kfree((void *)i2c_priv->hwrng.priv);
>  }
> 
> -static const struct of_device_id atmel_sha204a_dt_ids[] __maybe_unused = {
> -	{ .compatible = "atmel,atsha204", },
> -	{ .compatible = "atmel,atsha204a", },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, atmel_sha204a_dt_ids);
> -
> -static const struct i2c_device_id atmel_sha204a_id[] = {
> -	{ "atsha204" },
> -	{ "atsha204a" },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(i2c, atmel_sha204a_id);
> -
>  static struct i2c_driver atmel_sha204a_driver = {
>  	.probe			= atmel_sha204a_probe,
>  	.remove			= atmel_sha204a_remove,

Re: [PATCH] crypto: atmel-sha204a - drop hwrng quality reduction for ATSHA204A

[Thorsten Blum]

On Tue, Apr 28, 2026 at 07:58:39AM +0200, Ard Biesheuvel wrote:
> Hi Thorsten,
> 
> On Mon, 27 Apr 2026, at 14:40, Thorsten Blum wrote:
> > Commit 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to
> > lowest possible") reduced the hwrng quality to 1 based on a review by
> > Bill Cox [1]. However, despite its title, the review only tested the
> > ATSHA204, not the ATSHA204A.
> >
> > In the same thread, Atmel engineer Landon Cox wrote "this behavior has
> > been eliminated entirely"[2] in the ATSHA204A and "this problem does not
> > affect the ATECC108 or the ATECC108A (or the ATSHA204A)"[3].
> >
> > According to the official ATSHA204A datasheet [4], the device contains a
> > high-quality hardware RNG that combines its output with an internal seed
> > value stored in EEPROM or SRAM to generate random numbers. The device
> > also implements all security functions using SHA-256, and the driver
> > uses the chip's Random command in seed-update mode.
> >
> > Keep 'quality = 1' for ATSHA204, but drop the explicit hwrng quality
> > reduction for ATSHA204A and fall back to the hwrng core default.
> >
> 
> Interesting! Thanks for digging this up.
> 
> > [1] 
> > https://www.metzdowd.com/pipermail/cryptography/2014-December/023858.html
> > [2] 
> > https://www.metzdowd.com/pipermail/cryptography/2014-December/023852.html
> > [3] 
> > https://www.metzdowd.com/pipermail/cryptography/2014-December/023886.html
> > [4] 
> > https://ww1.microchip.com/downloads/en/DeviceDoc/ATSHA204A-Data-Sheet-40002025A.pdf
> >
> > Fixes: 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to 
> > lowest possible")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > ---
> >  drivers/crypto/atmel-sha204a.c | 40 ++++++++++++++++++----------------
> >  1 file changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/crypto/atmel-sha204a.c b/drivers/crypto/atmel-sha204a.c
> > index dbb39ed0cea1..df69fb190e52 100644
> > --- a/drivers/crypto/atmel-sha204a.c
> > +++ b/drivers/crypto/atmel-sha204a.c
> > @@ -19,6 +19,25 @@
> >  #include <linux/workqueue.h>
> >  #include "atmel-i2c.h"
> > 
> > +enum atmel_sha204a_variant {
> > +	ATSHA204 = 1,
> > +	ATSHA204A,
> > +};
> > +
> 
> I agree that setting quality to '1' is only appropriate for the ATSHA204
> but this looks a bit clunky to me.

Moving the declarations to the top was also a leftover from a previous
approach and not necessary anymore. I'll send a v2.

Thanks,
Thorsten