[Pkg-gnupg-commit] [gnupg2] 15/116: scd: Fix a race condition for new_reader_slot.
Daniel Kahn Gillmor
dkg at fifthhorseman.net
Tue Jan 24 04:40:49 UTC 2017
This is an automated email from the git hooks/post-receive script.
dkg pushed a commit to branch master
in repository gnupg2.
commit c48cf7e32ffa02ebdf00265543344c611bef0431
Author: NIIBE Yutaka <gniibe at fsij.org>
Date: Thu Dec 29 10:07:43 2016 +0900
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 at fsij.org>
---
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 177cd8e..d0b75c8 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 ba856af..3021cf7 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 a78d652..fa05475 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 38e3c40..74fed44 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 e18ebfe..22b17b4 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);
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-gnupg/gnupg2.git
More information about the Pkg-gnupg-commit
mailing list