Re: [PATCH] hwrng: core - Fix wrong quality calculation at hw rng registration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 21/06/2024 11:54, Harald Freudenberger wrote:
> When there are rng sources registering at the hwrng core via
> hwrng_register() a struct hwrng is delivered. There is a quality
> field in there which is used to decide which of the registered
> hw rng sources will be used by the hwrng core.
> 
> With commit 16bdbae39428 ("hwrng: core - treat default_quality as
> a maximum and default to 1024") there came in a new default of
> 1024 in case this field is empty and all the known hw rng sources
> at that time had been reworked to not fill this field and thus
> use the default of 1024.
> 
> The code choosing the 'better' hw rng source during registration
> of a new hw rng source has never been adapted to this and thus
> used 0 if the hw rng implementation does not fill the quality field.
> So when two rng sources register, one with 0 (meaning 1024) and
> the other one with 999, the 999 hw rng will be chosen.
> 
> This patch simple takes into account that a quality field value
> of 0 is to be treated as 1024 and then the decision about which
> hw rng to use works as expected.
> 
> Tested on s390 with two hardware rng sources: crypto cards and
> trng true random generator device driver.
> 
> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
> 
> Reported-by: Christian Rund <Christian.Rund@xxxxxxxxxx>
> Fixes: 16bdbae39428 ("hwrng: core - treat default_quality as a maximum and default to 1024")
> ---
>  drivers/char/hw_random/core.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 4084df65c9fa..993b8a1f1d19 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -525,6 +525,7 @@ static int hwrng_fillfn(void *unused)
>  
>  int hwrng_register(struct hwrng *rng)
>  {
> +	unsigned short rng_quality, cur_quality;

In my opinion, we no not need these variables.

>  	int err = -EINVAL;
>  	struct hwrng *tmp;
>  
> @@ -545,8 +546,14 @@ int hwrng_register(struct hwrng *rng)
>  	complete(&rng->cleanup_done);
>  	init_completion(&rng->dying);
>  
> +	/* Quality field not set in struct hwrng means 1024 */
> +	rng_quality = rng->quality ? rng->quality : 1024;

Please use the shortcut "(a) ?: (b)" for "(a) ? (a) : (b)", also remove non-necessary parenthesis.

	rng_quality = rng->quality ?: 1024;

Because this variable is only used once, you can also change it directly in the if statement below.

> +	cur_quality = current_rng ?
> +		(current_rng->quality ? current_rng->quality : 1024) :
> +		0;
> +

This one is not necessary. The quality field of current_rng is has been updated already by the hwrng_init() function. 

>  	if (!current_rng ||
> -	    (!cur_rng_set_by_user && rng->quality > current_rng->quality)) {
> +	    (!cur_rng_set_by_user && rng_quality > cur_quality)) {

Unfortunately, the quality field of rng is read here, before the quality field is updated by hwrng_init().
Maybe we can use the following:

  	if (!current_rng ||
	    (!cur_rng_set_by_user && (rng->quality ?: 1024) > current_rng->quality)) {

>  		/*
>  		 * Set new rng as current as the new rng source
>  		 * provides better entropy quality and was not

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@xxxxxxxxxxxxx




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]
  Powered by Linux