From c48cf7e32ffa02ebdf00265543344c611bef0431 Mon Sep 17 00:00:00 2001 From: NIIBE Yutaka Date: Thu, 29 Dec 2016 10:07:43 +0900 Subject: [PATCH] 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 --- scd/apdu.c | 67 +++++++++++++++++++++++++++++++++----------------- scd/apdu.h | 2 ++ scd/app.c | 43 ++++++++++++++++---------------- scd/scdaemon.c | 7 +++++- scd/scdaemon.h | 2 +- 5 files changed, 74 insertions(+), 47 deletions(-) diff --git a/scd/apdu.c b/scd/apdu.c index 177cd8e4b..d0b75c872 100644 --- a/scd/apdu.c +++ b/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 && reader == -1) + if (!reader_table[i].used) + { 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; +} diff --git a/scd/apdu.h b/scd/apdu.h index ba856af9e..3021cf7a5 100644 --- a/scd/apdu.h +++ b/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, diff --git a/scd/app.c b/scd/app.c index a78d6525a..fa0547567 100644 --- a/scd/app.c +++ b/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; - - err = npth_mutex_lock (&app->lock); - if (err) + if (npth_mutex_lock (&app->lock)) { + gpg_error_t err = gpg_error_from_syserror (); log_error ("failed to acquire APP lock for %p: %s\n", - app, strerror (err)); - return gpg_error_from_errno (err); + app, gpg_strerror (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 (err) - log_error ("failed to release APP lock for %p: %s\n", - app, strerror (err)); + if (npth_mutex_unlock (&app->lock)) + { + gpg_error_t err = gpg_error_from_syserror (); + 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; - int err; + gpg_error_t err; - if (!initialized) + if (npth_mutex_init (&app_list_lock, NULL)) { - err = npth_mutex_init (&app_list_lock, NULL); - if (!err) - initialized = 1; + err = gpg_error_from_syserror (); + log_error ("app: error initializing mutex: %s\n", gpg_strerror (err)); + return err; } + + return apdu_init (); } diff --git a/scd/scdaemon.c b/scd/scdaemon.c index 38e3c40c4..74fed4454 100644 --- a/scd/scdaemon.c +++ b/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); diff --git a/scd/scdaemon.h b/scd/scdaemon.h index e18ebfe4c..22b17b49e 100644 --- a/scd/scdaemon.h +++ b/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);