[PATCH v2] DRBG: simplify ordering of linked list in drbg_ctr_df

[PATCH v2] DRBG: simplify ordering of linked list in drbg_ctr_df

[Stephan Mueller]

This patch superseeds and replaces the initial fix submitted with [1].
After careful analysis of the code, the anticipated NULL pointer
deference is caught in drbg_ctr_update which only invokes drbg_ctr_df
when addtl is not NULL.

This patch is tested with CAVS testing and the test set provided in [2].

[1] https://lkml.org/lkml/2014/6/21/70
[2] http://www.chronox.de/drbg.html

Stephan Mueller (1):
  DRBG: simplify ordering of linked list in drbg_ctr_df

 crypto/drbg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
1.9.3

[PATCH v2] DRBG: simplify ordering of linked list in drbg_ctr_df

[Stephan Mueller]

As reported by a static code analyzer, the code for the ordering of
the linked list can be simplified.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index faaa2ce..99fa8f8 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -516,13 +516,13 @@ static int drbg_ctr_df(struct drbg_state *drbg,
 	S2.next = addtl;
 
 	/*
-	 * splice in addtl between S2 and S4 -- we place S4 at the end of the
-	 * input data chain
+	 * Splice in addtl between S2 and S4 -- we place S4 at the end
+	 * of the input data chain. As this code is only triggered when
+	 * addtl is not NULL, no NULL checks are necessary.
 	 */
 	tempstr = addtl;
-	for (; NULL != tempstr; tempstr = tempstr->next)
-		if (NULL == tempstr->next)
-			break;
+	while (tempstr->next)
+		tempstr = tempstr->next;
 	tempstr->next = &S4;
 
 	/* 10.4.2 step 9 */
-- 
1.9.3

Re: [PATCH v2] DRBG: simplify ordering of linked list in drbg_ctr_df

[Herbert Xu]

On Mon, Jun 23, 2014 at 09:11:29AM +0200, Stephan Mueller wrote:
> As reported by a static code analyzer, the code for the ordering of
> the linked list can be simplified.
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/drbg.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index faaa2ce..99fa8f8 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -516,13 +516,13 @@ static int drbg_ctr_df(struct drbg_state *drbg,
>  	S2.next = addtl;
>  
>  	/*
> -	 * splice in addtl between S2 and S4 -- we place S4 at the end of the
> -	 * input data chain
> +	 * Splice in addtl between S2 and S4 -- we place S4 at the end
> +	 * of the input data chain. As this code is only triggered when
> +	 * addtl is not NULL, no NULL checks are necessary.
>  	 */
>  	tempstr = addtl;
> -	for (; NULL != tempstr; tempstr = tempstr->next)
> -		if (NULL == tempstr->next)
> -			break;
> +	while (tempstr->next)
> +		tempstr = tempstr->next;
>  	tempstr->next = &S4;

This is still broken.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] DRBG: simplify ordering of linked list in drbg_ctr_df

[Herbert Xu]

On Wed, Jun 25, 2014 at 05:08:28PM +0800, Herbert Xu wrote:
> On Mon, Jun 23, 2014 at 09:11:29AM +0200, Stephan Mueller wrote:
> > As reported by a static code analyzer, the code for the ordering of
> > the linked list can be simplified.
> > 
> > Reported-by: kbuild test robot <fengguang.wu@intel.com>
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> > ---
> >  crypto/drbg.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/crypto/drbg.c b/crypto/drbg.c
> > index faaa2ce..99fa8f8 100644
> > --- a/crypto/drbg.c
> > +++ b/crypto/drbg.c
> > @@ -516,13 +516,13 @@ static int drbg_ctr_df(struct drbg_state *drbg,
> >  	S2.next = addtl;
> >  
> >  	/*
> > -	 * splice in addtl between S2 and S4 -- we place S4 at the end of the
> > -	 * input data chain
> > +	 * Splice in addtl between S2 and S4 -- we place S4 at the end
> > +	 * of the input data chain. As this code is only triggered when
> > +	 * addtl is not NULL, no NULL checks are necessary.
> >  	 */
> >  	tempstr = addtl;
> > -	for (; NULL != tempstr; tempstr = tempstr->next)
> > -		if (NULL == tempstr->next)
> > -			break;
> > +	while (tempstr->next)
> > +		tempstr = tempstr->next;
> >  	tempstr->next = &S4;
> 
> This is still broken.

OK I take that back.  As addtl is not NULL neither version will
do a NULL derference.  But I will apply your cleanup patch anyway.

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

Re: [PATCH v2] DRBG: simplify ordering of linked list in drbg_ctr_df

[Stephan Mueller]

Am Donnerstag, 26. Juni 2014, 14:45:42 schrieb Herbert Xu:

Hi Herbert,

>On Wed, Jun 25, 2014 at 05:08:28PM +0800, Herbert Xu wrote:
>> On Mon, Jun 23, 2014 at 09:11:29AM +0200, Stephan Mueller wrote:
>> > As reported by a static code analyzer, the code for the ordering of
>> > the linked list can be simplified.
>> > 
>> > Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
>> > ---
>> > 
>> >  crypto/drbg.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/crypto/drbg.c b/crypto/drbg.c
>> > index faaa2ce..99fa8f8 100644
>> > --- a/crypto/drbg.c
>> > +++ b/crypto/drbg.c
>> > @@ -516,13 +516,13 @@ static int drbg_ctr_df(struct drbg_state
>> > *drbg,
>> > 
>> >  	S2.next = addtl;
>> >  	
>> >  	/*
>> > 
>> > -	 * splice in addtl between S2 and S4 -- we place S4 at the end 
of
>> > the -	 * input data chain
>> > +	 * Splice in addtl between S2 and S4 -- we place S4 at the end
>> > +	 * of the input data chain. As this code is only triggered when
>> > +	 * addtl is not NULL, no NULL checks are necessary.
>> > 
>> >  	 */
>> >  	
>> >  	tempstr = addtl;
>> > 
>> > -	for (; NULL != tempstr; tempstr = tempstr->next)
>> > -		if (NULL == tempstr->next)
>> > -			break;
>> > +	while (tempstr->next)
>> > +		tempstr = tempstr->next;
>> > 
>> >  	tempstr->next = &S4;
>> 
>> This is still broken.
>
>OK I take that back.  As addtl is not NULL neither version will
>do a NULL derference.  But I will apply your cleanup patch anyway.

When I wrote my first patch considering the NULL pointer, I was already 
wondering why during my tests I did not observe any crasher. In case the 
NULL pointer dereference would have been real, it would need to have 
crashed when pulling random bytes via the kernel crypto API -- I have a 
test that iterates over all DRBG types, instantiates them and pulls up 
to 100,000 bytes.

If the NULL pointer dereference would have been real, the following call 
sequences triggered by normal kernel crypto API usage should have 
triggered it, because they all set addtl to NULL.

crypto_rng_get_bytes
--> drbg_kcapi_random with slen >0
--> drbg_generate_long(drbg, rdata, dlen, NULL);
--> drbg_generate(drbg, rdata, dlen, NULL);
--> drbg_ctr_generate(..., NULL)

And here, the following is only called when addtl is not NULL
--> drbg_ctr_update
--> drbg_ctr_df


Ciao
Stephan