From 510e1b802747aa6d1eb1b078ed64f1de3caa845d Mon Sep 17 00:00:00 2001 From: Holger Dengler Date: Fri, 17 Jun 2011 11:14:56 +0200 Subject: [PATCH 1/3] Description: libica: Fix temporary buffer allocation in ica_get_version(). Symptom: Sometimes calling ica_get_version() ends up in a segmentation fault. Problem: In function ica_get_version() the version information is copied to a temporary buffer. The allocated buffer is to small to hold the complete version information and the trailing termination character. Solution: Adjust the allocation of the temporary buffer as large as the version information plus the string terminator. --- src/ica_api.c | 59 ++++++++++++++++++++++++++++++-------------------------- 1 files changed, 32 insertions(+), 27 deletions(-) diff --git a/src/ica_api.c b/src/ica_api.c index a853ce1..e44678a 100644 --- a/src/ica_api.c +++ b/src/ica_api.c @@ -36,6 +36,8 @@ #define DEFAULT2_CRYPT_DEVICE "/dev/z90crypt" #define DEFAULT3_CRYPT_DEVICE "/dev/zcrypt" +#define MAX_VERSION_LENGTH 16 + static unsigned int check_des_parms(unsigned int mode, unsigned long data_length, const unsigned char *in_data, @@ -1044,48 +1046,51 @@ unsigned int ica_aes_cmac(const unsigned char *message, unsigned long message_le unsigned int ica_get_version(libica_version_info *version_info) { - /* - * We expect the libica version information in the format x.y.z - * defined in the macro VERSION as part of the build process. - */ -#ifndef VERSION - return EIO; -#endif - - int length = strlen(VERSION); +#ifdef VERSION + int length; int rc; - int i = 1; + int i; char *pch; + char *saveptr; if (version_info == NULL) { return EINVAL; } - char buffer[length]; - rc = sprintf(buffer, "%s", VERSION); + length = strnlen(VERSION, MAX_VERSION_LENGTH); + char buffer[length+1]; + + rc = snprintf(buffer, (length+1), "%s", VERSION); if (rc <= 0) { - return 1; + return EIO; } - pch = strtok(buffer, "."); - - while (pch != NULL) { - if (i == 1) + for (pch = strtok_r(buffer, ".", &saveptr), i = 1; + pch != NULL; + pch = strtok_r(NULL, ".", &saveptr), i++) + { + switch(i) { + case 1: version_info->major_version = atoi(pch); - if (i == 2) + break; + case 2: version_info->minor_version = atoi(pch); - if (i == 3) + break; + case 3: version_info->fixpack_version = atoi(pch); - if (i > 3) - return 1; - - pch = strtok(NULL, "."); - i++; + break; + default: + return EIO; + } } - if (i < 3) { - return 1; - } + if (i < 3) + return EIO; return 0; +#else + /* We expect the libica version information in the format x.y.z + * defined in the macro VERSION as part of the build process. */ + return EIO; +#endif } -- 1.7.4.4 From cc1b8bcfe9dd4a119e726380626b372175595980 Mon Sep 17 00:00:00 2001 From: Holger Dengler Date: Fri, 17 Jun 2011 11:20:27 +0200 Subject: [PATCH 2/3] Fix result/error handling in testcase for ica_get_version(). --- src/tests/libica_get_version.c | 46 +++++++++++++++++++++++---------------- 1 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/tests/libica_get_version.c b/src/tests/libica_get_version.c index 086e228..63f8c55 100644 --- a/src/tests/libica_get_version.c +++ b/src/tests/libica_get_version.c @@ -14,37 +14,45 @@ */ #include #include +#include #include "ica_api.h" int main(int argc, char **argv) { libica_version_info version_info; int rc; + int failed = 0; - printf("Testing libica API ica_get_version(). Invalid input (NULL).\n"); - + printf("Testing libica API ica_get_version() w/ invalid input (NULL).\n"); rc = ica_get_version(NULL); - - if (rc == 0) { - printf("OK. Not expected (RC=%d).\n", rc); - } else { - printf("Error. Expected(RC=%d).\n", rc); + if (rc == EINVAL) + printf("Test successful"); + else { + printf("Test failed"); + failed++; } + printf(" (rc=%d, expected: %d)\n", rc, EINVAL); - printf("Testing libica API ica_get_version_(). Valid input.\n"); - + printf("Testing libica API ica_get_version_() w/ valid input.\n"); rc = ica_get_version(&version_info); - - if (rc == 0) { - printf("OK. Expected (RC=%d).\n", rc); - } else { - printf("Error. Not expected (RC=%d).\n", rc); - return rc; + if (rc == 0) + printf("Test successful"); + else { + printf("Test failed"); + failed++; } + printf(" (rc=%d, expected: %d)\n", rc, 0); - printf("Major_version:%d\n", version_info.major_version); - printf("Minor_version:%d\n", version_info.minor_version); - printf("Fixpack_version:%d\n", version_info.fixpack_version); + printf("Major_version:%d, minor_version %d, fixpack_version %d\n", + version_info.major_version, + version_info.minor_version, + version_info.fixpack_version); - return 0; + if (failed) { + printf("Failed tests: %d\n", failed); + return 1; + } else { + printf("All tests completed sucessfully\n"); + return 0; + } } -- 1.7.4.4 From 51474af2b74735931e89864c392a407499f2f3a6 Mon Sep 17 00:00:00 2001 From: Holger Dengler Date: Thu, 30 Jun 2011 12:46:24 +0200 Subject: [PATCH 3/3] [PATCH]: synchronize shared memory ref counting Description: libica: synchronize shared memory reference counting for library statisitcs Symptom: sshd daemon blocks all login tries. Other processes, which accessing libica, do not run correctly. Problem: Libica setup a shared memory for counting some statistics across all processes and threads using the library. The locking and reference counting is re- ordered by compiler optimization. Beside that, the lock is not released if the last process unloads the library. Solution: Insert barriers to synchronize shared memory reference counting. Release lock, if last process unloads the library. Use non-blocking lock, which may have the side effect, that on locking problems the statistics counting is disabled instead of blocking processes, which using libica. --- src/icastats_shared.c | 67 +++++++++++++++++++++++++++++++++++-------------- 1 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/icastats_shared.c b/src/icastats_shared.c index b044dea..ead1067 100644 --- a/src/icastats_shared.c +++ b/src/icastats_shared.c @@ -6,6 +6,7 @@ /** * Authors: Christian Maaser + * Holger Dengler * * Copyright IBM Corp. 2009, 2011 */ @@ -23,9 +24,11 @@ typedef struct statis_entry { uint32_t software; } stats_entry_t; -stats_entry_t *stats = 0; -int *stats_ref_counter = 0; -int stats_shm_handle = 0; +#define NOT_INITIALIZED (-1) + +stats_entry_t *stats = NULL; +volatile int *stats_ref_counter = NULL; +volatile int stats_shm_handle = NOT_INITIALIZED; void atomic_add(int *x, int i) { @@ -43,24 +46,32 @@ void atomic_add(int *x, int i) int stats_mmap() { + int local_stats_shm_handle; /* Use flock to avoid races between open and close of shm by different * processes. Put reference counter into shm to check how much * processes are accesing the shm. Additionaly a global and a local * handle for the shm are used to prevend different threads from * overriding their shm handle one another. + * Non-blocking flocks are used. This may end-up in disabled statistics + * instead of locking each process using libica. */ - if (!stats) { - int local_stats_shm_handle = shm_open(STATS_SHM_ID, O_CREAT | O_RDWR, - S_IRUSR | S_IWUSR | S_IRGRP | - S_IWGRP | S_IROTH | S_IWOTH); - if (local_stats_shm_handle == -1) + if (stats == NULL) { + local_stats_shm_handle = shm_open(STATS_SHM_ID, O_CREAT | O_RDWR, + S_IRUSR | S_IWUSR | S_IRGRP | + S_IWGRP | S_IROTH | S_IWOTH); + if (local_stats_shm_handle == NOT_INITIALIZED) return -1; if (ftruncate(local_stats_shm_handle, STATS_SHM_SIZE) == -1) return -1; - - if (flock(local_stats_shm_handle, LOCK_EX) == -1) + if (flock(local_stats_shm_handle, LOCK_EX | LOCK_NB) == -1) return -1; - if (stats_shm_handle != 0) { + + /* This barrier prohibits re-ordering of locking and + * reference counting due to compiler optimizations. */ + asm volatile ("" : : : "memory"); + + if (stats_shm_handle != NOT_INITIALIZED) { + /* Another process/thread won the race, give up. */ flock(local_stats_shm_handle, LOCK_UN); return 0; } @@ -68,17 +79,22 @@ int stats_mmap() PROT_WRITE, MAP_SHARED, local_stats_shm_handle, 0); if (stats_ref_counter == MAP_FAILED) { - stats_ref_counter = 0; + stats_ref_counter = NULL; flock(local_stats_shm_handle, LOCK_UN); return -1; } ++(*stats_ref_counter); - stats = (stats_entry_t *) (stats_ref_counter + 1); + stats = (stats_entry_t *) (stats_ref_counter + sizeof(stats_ref_counter)); stats_shm_handle = local_stats_shm_handle; flock(local_stats_shm_handle, LOCK_UN); } else { - if (flock(stats_shm_handle, LOCK_EX) == -1) + if (flock(stats_shm_handle, LOCK_EX | LOCK_NB) == -1) return -1; + + /* This barrier prohibits re-ordering of locking and + * reference counting due to compiler optimizations. */ + asm volatile ("" : : : "memory"); + ++(*stats_ref_counter); flock(stats_shm_handle, LOCK_UN); } @@ -88,13 +104,22 @@ int stats_mmap() void stats_munmap() { - if (!stats) + int tmp_handle; + if (stats == NULL) return; - if (flock(stats_shm_handle, LOCK_EX) == -1) + if (flock(stats_shm_handle, LOCK_EX | LOCK_NB) == -1) return; + + /* This barrier prohibits re-ordering of locking and + * reference counting due to compiler optimizations. */ + asm volatile ("" : : : "memory"); + if (--(*stats_ref_counter) == 0) { - munmap(stats_ref_counter, STATS_SHM_SIZE); + munmap((void *)stats_ref_counter, STATS_SHM_SIZE); + tmp_handle = stats_shm_handle; + stats_shm_handle = NOT_INITIALIZED; + flock(tmp_handle, LOCK_UN); shm_unlink(STATS_SHM_ID); stats = 0; } @@ -104,7 +129,7 @@ void stats_munmap() uint32_t stats_query(stats_fields_t field, int hardware) { - if (!stats) + if (stats == NULL) return 0; if (hardware) @@ -115,7 +140,7 @@ uint32_t stats_query(stats_fields_t field, int hardware) void stats_increment(stats_fields_t field, int hardware) { - if (!stats) + if (stats == NULL) return; if (hardware) @@ -127,6 +152,10 @@ void stats_increment(stats_fields_t field, int hardware) void stats_reset() { unsigned int i; + + if (stats == NULL) + return; + for (i = 0; i != ICA_NUM_STATS; ++i) { stats[i].hardware = 0; stats[i].software = 0; -- 1.7.4.4