From 07bf6f52c1e3317a12e53b523199fd9474831aa1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?=
 <manuel.pegourie-gonnard@arm.com>
Date: Thu, 16 Mar 2017 17:21:38 +0100
Subject: [PATCH] Tune T ownership code + comments

Don't miss the little code changes among all those comments change :)
---
 library/ecp.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/library/ecp.c b/library/ecp.c
index 71fb314cc..f852c9988 100644
--- a/library/ecp.c
+++ b/library/ecp.c
@@ -1618,8 +1618,17 @@ static unsigned char ecp_pick_window_size( const mbedtls_ecp_group *grp,
 }
 
 /*
- * Multiplication using the comb method,
- * for curves in short Weierstrass form
+ * Multiplication using the comb method - for curves in short Weierstrass form
+ *
+ * This function is mainly responsible for administrative work:
+ * - managing the restart context if enabled
+ * - managing the table of precomputed points (passed between the above two
+ *   functions): allocation, computation, ownership tranfer, freeing.
+ *
+ * It delegates the actual arithmetic work to:
+ *      ecp_precompute_comb() and ecp_mul_comb_with_precomp()
+ *
+ * See comments on ecp_comb_recode_core() regarding the computation strategy.
  */
 static int ecp_mul_comb( mbedtls_ecp_group *grp, mbedtls_ecp_point *R,
                          const mbedtls_mpi *m, const mbedtls_ecp_point *P,
@@ -1657,7 +1666,7 @@ static int ecp_mul_comb( mbedtls_ecp_group *grp, mbedtls_ecp_point *R,
         MBEDTLS_MPI_CHK( mbedtls_ecp_copy( &grp->rs->P, P ) );
     }
 
-    /* new start for ops counts */
+    /* reset ops count for this call */
     if( grp->rs != NULL )
         grp->rs->ops_done = 0;
 #endif
@@ -1676,6 +1685,8 @@ static int ecp_mul_comb( mbedtls_ecp_group *grp, mbedtls_ecp_point *R,
     /* Pre-computed table: do we have it already for the base point? */
     if( p_eq_g && grp->T != NULL )
     {
+        /* second pointer to the same table
+         * no ownership transfer as other threads might be using T too */
         T = grp->T;
         T_ok = 1;
     }
@@ -1684,7 +1695,7 @@ static int ecp_mul_comb( mbedtls_ecp_group *grp, mbedtls_ecp_point *R,
     /* Pre-computed table: do we have one in progress? complete? */
     if( grp->rs != NULL && grp->rs->T != NULL && T == NULL )
     {
-        /* transfer "ownership" of T from rs to local function */
+        /* transfer ownership of T from rs to local function */
         T = grp->rs->T;
         grp->rs->T = NULL;
         grp->rs->T_size = 0;
@@ -1714,6 +1725,7 @@ static int ecp_mul_comb( mbedtls_ecp_group *grp, mbedtls_ecp_point *R,
         {
             grp->T = T;
             grp->T_size = pre_len;
+            /* now have two pointers to the same table */
         }
     }
 
@@ -1724,17 +1736,23 @@ static int ecp_mul_comb( mbedtls_ecp_group *grp, mbedtls_ecp_point *R,
 
 cleanup:
 
+    /* does T belong to the group? */
+    if( T == grp->T )
+        T = NULL;
+
+    /* does T belong to the restart context? */
 #if defined(MBEDTLS_ECP_EARLY_RETURN)
-    if( grp->rs != NULL && ret == MBEDTLS_ERR_ECP_IN_PROGRESS && T != grp->T )
+    if( grp->rs != NULL && ret == MBEDTLS_ERR_ECP_IN_PROGRESS && T != NULL )
     {
-        /* transfer "ownership" of T from local function to rs */
+        /* transfer ownership of T from local function to rs */
         grp->rs->T_size = pre_len;
         grp->rs->T = T;
         T = NULL;
     }
 #endif
 
-    if( T != NULL && ! p_eq_g )
+    /* did T belong to us? then let's destroy it! */
+    if( T != NULL )
     {
         for( i = 0; i < pre_len; i++ )
             mbedtls_ecp_point_free( &T[i] );
@@ -1745,9 +1763,11 @@ cleanup:
 #if defined(MBEDTLS_ECP_EARLY_RETURN)
     if( ret != MBEDTLS_ERR_ECP_IN_PROGRESS )
 #endif
+    /* prevent caller from using invalid value */
     if( ret != 0 )
         mbedtls_ecp_point_free( R );
 
+    /* clear restart context when not in progress (done or error) */
 #if defined(MBEDTLS_ECP_EARLY_RETURN)
     if( grp->rs != NULL && ret != MBEDTLS_ERR_ECP_IN_PROGRESS ) {
         ecp_restart_free( grp->rs );