linux-kernel - Re: [PATCH v3 4/6] crypto: drbg

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] [day] [month] [year] [list]

Date:	Fri, 01 May 2015 06:15:58 +0200
From:	Stephan Mueller <smueller@...onox.de>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	pebolle@...cali.nl, andreas.steffen@...ongswan.org, tytso@....edu,
	sandyinchina@...il.com, linux-kernel@...r.kernel.org,
	linux-crypto@...r.kernel.org
Subject: Re: [PATCH v3 4/6] crypto: drbg - add async seeding operation

Am Freitag, 1. Mai 2015, 11:13:31 schrieb Herbert Xu:

Hi Herbert,

>On Tue, Apr 28, 2015 at 05:00:03AM +0200, Stephan Mueller wrote:
>> @@ -1081,6 +1115,11 @@ static int drbg_seed(struct drbg_state *drbg, struct
>> drbg_string *pers,> 
>>  		return -EINVAL;
>>  	
>>  	}
>> 
>> +	/* cancel any previously invoked seeding */
>> +	mutex_unlock(&drbg->drbg_mutex);
>> +	drbg_async_work_cancel(&drbg->seed_work);
>> +	mutex_lock(&drbg->drbg_mutex);
>
>This seems dangerous and unnecessary.  Releasing and reacquiring
>the locks may invalidate previous checks.  Even if it doesn't
>matter today if somebody modifies the callers later on this could
>explode.

Agreed.
>
>You can easily remove this by making get_blocking_random_bytes_cb
>idempotent, i.e., do nothing if the work is already queued, which
>is what it would do anyway if you simply move the INIT_WORK out of
>it.

As the get_blocking_random_bytes_cb fully sets up the random_work data 
structure, I think INIT_WORK should be left in there to have a nice and easy 
API. Otherwise either a new call would need to be added to random.c. The 
caller is not able to invoke INIT_WORK himself as the worker function is 
static.

However, what about simply checking if rw->work is NULL and only then 
performing the INIT_WORK? In that case then, I guess that all the members of 
random_walk in get_blocking_random_bytes_cb should only be filled in if 
INIT_WORK is to be called as otherwise a race may occur: 
get_blocking_random_bytes_work already performs its operation on the data in 
the supplied random_work and in the middle of that work, and then we would 
change it with a new call to get_blocking_random_bytes_cb.

Ciao
Stephan
--
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/