scd: Fix a race condition for new_reader_slot.

* scd/apdu.c (reader_table_lock, apdu_init): New.
(new_reader_slot): Serialize by reader_table_lock.
* scd/app.c (lock_app, unlock_app, app_new_register): Fix error code
usage.
(initialize_module_command): Call apdu_init.
* scd/scdaemon.c (main): Handle error for initialize_module_command.

--

This is a long standing bug.  There are two different things; The
serialization of allocating a new SLOT, and the serialization of using
the SLOT.  The latter was implemented in new_reader_slot by lock_slot.
However, the former was not done.  Thus, there was a possible race where
a same SLOT is allocated to multiple threads.

Signed-off-by: NIIBE Yutaka <gniibe@fsij.org>

scd/apdu.c

@@ -144,7 +144,6 @@ struct reader_table_s {
                               not yet been read; i.e. the card is not
                               ready for use. */
 #ifdef USE_NPTH
-  int lock_initialized;
   npth_mutex_t lock;
 #endif
 };
@@ -153,6 +152,10 @@ typedef struct reader_table_s *reader_table_t;
 /* A global table to keep track of active readers. */
 static struct reader_table_s reader_table[MAX_READER];
 
+#ifdef USE_NPTH
+static npth_mutex_t reader_table_lock;
+#endif
+
 
 /* ct API function pointer. */
 static char (* DLSTDCALL CT_init) (unsigned short ctn, unsigned short Pn);
@@ -424,35 +427,29 @@ static int
 new_reader_slot (void)
 {
   int i, reader = -1;
-  int err;
 
+  npth_mutex_lock (&reader_table_lock);
   for (i=0; i < MAX_READER; i++)
-    {
+    if (!reader_table[i].used)
-      if (!reader_table[i].used && reader == -1)
+      {
         reader = i;
-    }
+        reader_table[reader].used = 1;
+        break;
+      }
+  npth_mutex_unlock (&reader_table_lock);
+
   if (reader == -1)
     {
       log_error ("new_reader_slot: out of slots\n");
       return -1;
     }
-#ifdef USE_NPTH
+
-  if (!reader_table[reader].lock_initialized)
-    {
-      err = npth_mutex_init (&reader_table[reader].lock, NULL);
-      if (err)
-        {
-          log_error ("error initializing mutex: %s\n", strerror (err));
-          return -1;
-        }
-      reader_table[reader].lock_initialized = 1;
-    }
-#endif /*USE_NPTH*/
   if (lock_slot (reader))
     {
-      log_error ("error locking mutex: %s\n", strerror (errno));
+      reader_table[reader].used = 0;
       return -1;
     }
+
   reader_table[reader].connect_card = NULL;
   reader_table[reader].disconnect_card = NULL;
   reader_table[reader].close_reader = NULL;
@@ -465,7 +462,6 @@ new_reader_slot (void)
   reader_table[reader].pinpad_verify = pcsc_pinpad_verify;
   reader_table[reader].pinpad_modify = pcsc_pinpad_modify;
 
-  reader_table[reader].used = 1;
   reader_table[reader].is_t0 = 1;
   reader_table[reader].is_spr532 = 0;
   reader_table[reader].pinpad_varlen_supported = 0;
@@ -650,7 +646,6 @@ static int
 close_ct_reader (int slot)
 {
   CT_close (slot);
-  reader_table[slot].used = 0;
   return 0;
 }
 
@@ -1435,7 +1430,6 @@ close_pcsc_reader_direct (int slot)
   pcsc_release_context (reader_table[slot].pcsc.context);
   xfree (reader_table[slot].rdrname);
   reader_table[slot].rdrname = NULL;
-  reader_table[slot].used = 0;
   return 0;
 }
 #endif /*!NEED_PCSC_WRAPPER*/
@@ -2439,7 +2433,6 @@ close_ccid_reader (int slot)
 {
   ccid_close_reader (reader_table[slot].ccid.handle);
   reader_table[slot].rdrname = NULL;
-  reader_table[slot].used = 0;
   return 0;
 }
 
@@ -2670,7 +2663,6 @@ static int
 close_rapdu_reader (int slot)
 {
   rapdu_release (reader_table[slot].rapdu.handle);
-  reader_table[slot].used = 0;
   return 0;
 }
 
@@ -3180,10 +3172,12 @@ apdu_close_reader (int slot)
   if (reader_table[slot].close_reader)
     {
       sw = reader_table[slot].close_reader (slot);
+      reader_table[slot].used = 0;
       if (DBG_READER)
         log_debug ("leave: apdu_close_reader => 0x%x (close_reader)\n", sw);
       return sw;
     }
+  reader_table[slot].used = 0;
   if (DBG_READER)
     log_debug ("leave: apdu_close_reader => SW_HOST_NOT_SUPPORTED\n");
   return SW_HOST_NOT_SUPPORTED;
@@ -3203,6 +3197,7 @@ apdu_prepare_exit (void)
   if (!sentinel)
     {
       sentinel = 1;
+      npth_mutex_lock (&reader_table_lock);
       for (slot = 0; slot < MAX_READER; slot++)
         if (reader_table[slot].used)
           {
@@ -3211,6 +3206,7 @@ apdu_prepare_exit (void)
               reader_table[slot].close_reader (slot);
             reader_table[slot].used = 0;
           }
+      npth_mutex_unlock (&reader_table_lock);
       sentinel = 0;
     }
 }
@@ -4222,3 +4218,28 @@ apdu_get_reader_name (int slot)
 {
   return reader_table[slot].rdrname;
 }
+
+gpg_error_t
+apdu_init (void)
+{
+#ifdef USE_NPTH
+  gpg_error_t err;
+  int i;
+
+  if (npth_mutex_init (&reader_table_lock, NULL))
+    goto leave;
+
+  for (i = 0; i < MAX_READER; i++)
+    if (npth_mutex_init (&reader_table[i].lock, NULL))
+      goto leave;
+
+  /* All done well.  */
+  return 0;
+
+ leave:
+  err = gpg_error_from_syserror ();
+  log_error ("apdu: error initializing mutex: %s\n", gpg_strerror (err));
+  return err;
+#endif /*USE_NPTH*/
+  return 0;
+}

scd/apdu.h

@@ -84,6 +84,8 @@ enum {
 #define APDU_CARD_ACTIVE   (4)    /* Card is active.  */
 
 
+gpg_error_t apdu_init (void);
+
 /* Note, that apdu_open_reader returns no status word but -1 on error. */
 int apdu_open_reader (const char *portstr);
 int apdu_open_remote_reader (const char *portstr,

scd/app.c

@@ -58,14 +58,12 @@ print_progress_line (void *opaque, const char *what, int pc, int cur, int tot)
 static gpg_error_t
 lock_app (app_t app, ctrl_t ctrl)
 {
-  gpg_error_t err;
+  if (npth_mutex_lock (&app->lock))
-
-  err = npth_mutex_lock (&app->lock);
-  if (err)
     {
+      gpg_error_t err = gpg_error_from_syserror ();
       log_error ("failed to acquire APP lock for %p: %s\n",
-                 app, strerror (err));
+                 app, gpg_strerror (err));
-      return gpg_error_from_errno (err);
+      return err;
     }
 
   apdu_set_progress_cb (app->slot, print_progress_line, ctrl);
@@ -77,14 +75,14 @@ lock_app (app_t app, ctrl_t ctrl)
 static void
 unlock_app (app_t app)
 {
-  gpg_error_t err;
-
   apdu_set_progress_cb (app->slot, NULL, NULL);
 
-  err = npth_mutex_unlock (&app->lock);
+  if (npth_mutex_unlock (&app->lock))
-  if (err)
+    {
-    log_error ("failed to release APP lock for %p: %s\n",
+      gpg_error_t err = gpg_error_from_syserror ();
-               app, strerror (err));
+      log_error ("failed to release APP lock for %p: %s\n",
+                 app, gpg_strerror (err));
+    }
 }
 
 
@@ -194,11 +192,11 @@ app_new_register (int slot, ctrl_t ctrl, const char *name)
     }
 
   app->slot = slot;
-  err = npth_mutex_init (&app->lock, NULL);
+
-  if (err)
+  if (npth_mutex_init (&app->lock, NULL))
     {
       err = gpg_error_from_syserror ();
-      log_error ("error initializing mutex: %s\n", strerror (err));
+      log_error ("error initializing mutex: %s\n", gpg_strerror (err));
       xfree (app);
       return err;
     }
@@ -1057,16 +1055,17 @@ scd_update_reader_status_file (void)
    has to be done before a second thread is spawned.  We can't do the
    static initialization because Pth emulation code might not be able
    to do a static init; in particular, it is not possible for W32. */
-void
+gpg_error_t
 initialize_module_command (void)
 {
-  static int initialized;
+  gpg_error_t err;
-  int err;
 
-  if (!initialized)
+  if (npth_mutex_init (&app_list_lock, NULL))
     {
-      err = npth_mutex_init (&app_list_lock, NULL);
+      err = gpg_error_from_syserror ();
-      if (!err)
+      log_error ("app: error initializing mutex: %s\n", gpg_strerror (err));
-        initialized = 1;
+      return err;
     }
+
+  return apdu_init ();
 }

scd/scdaemon.c

@@ -640,7 +640,12 @@ main (int argc, char **argv )
 
   set_debug (debug_level);
 
-  initialize_module_command ();
+  if (initialize_module_command ())
+    {
+      log_error ("initialization failed\n");
+      cleanup ();
+      exit (1);
+    }
 
   if (gpgconf_list == 2)
     scd_exit (0);

scd/scdaemon.h

@@ -118,7 +118,7 @@ void scd_exit (int rc);
 const char *scd_get_socket_name (void);
 
 /*-- command.c --*/
-void initialize_module_command (void);
+gpg_error_t initialize_module_command (void);
 int  scd_command_handler (ctrl_t, int);
 void send_status_info (ctrl_t ctrl, const char *keyword, ...)
      GPGRT_ATTR_SENTINEL(1);