On (17/07/15 14:07), Lukas Slebodnik wrote:
On (16/07/15 17:07), Lukas Slebodnik wrote:
>On (16/07/15 13:46), Michal Židek wrote:
>>On 07/15/2015 06:42 PM, Lukas Slebodnik wrote:
>>>ehlo,
>>>
>>>reproducer:
>>> add user and few groups to ldap
>>> call id user
>>> remove one group
>>> authenticate
>>> call id user // removed groups should not appear in output
>>>
>>>LS
>>>
>>>
>>>0001-mmap_cache-Override-functions-for-initgr-mmap-cache.patch
>>>
>>>
>>> From 146337d5a3a15236b0a8c26e10d233e8cbc3a1d8 Mon Sep 17 00:00:00 2001
>>>From: Lukas Slebodnik<lslebodn(a)redhat.com>
>>>Date: Wed, 15 Jul 2015 17:47:29 +0200
>>>Subject: [PATCH] mmap_cache: "Override" functions for initgr mmap
cache
>>>
>>>Functions sss_mc_get_strs_offset and sss_mc_get_strs_len provides
>>>data about strings for individual memory caches (passwd, ...)
>>>Their are used in generic responder mmap cache code to find a record
>>>in mmap cache (sss_mc_find_record). Data provided from functions sss_mc_get_*
>>>are used for checking the validity of record. So in case of corrupted record
>>>the whole mmap cache can be invalidated.
>>>
>>>Functions sss_mc_get_strs_offset and sss_mc_get_strs_len did not provide
>>>data for initgroups mmap cache and therefore particular record could not be
>>>invalidated.
>>>
>>>Resolves:
>>>https://fedorahosted.org/sssd/ticket/2716
>>>---
>>> src/responder/nss/nsssrv_mmap_cache.c | 15 ++++++++++++---
>>> src/sss_client/nss_mc_initgr.c | 16 +++++++++++-----
>>> src/util/mmap_cache.h | 6 +++++-
>>> 3 files changed, 28 insertions(+), 9 deletions(-)
>>>
>>>diff --git a/src/responder/nss/nsssrv_mmap_cache.c
b/src/responder/nss/nsssrv_mmap_cache.c
>>>index
ebda8ac6fab3dd87f5a1d8e43717bf7a5b5a9878..6251ff93ce7f8bf473fc770f7aa3d2e0642395a3 100644
>>>--- a/src/responder/nss/nsssrv_mmap_cache.c
>>>+++ b/src/responder/nss/nsssrv_mmap_cache.c
>>>@@ -475,6 +475,9 @@ static errno_t sss_mc_get_strs_offset(struct sss_mc_ctx
*mcc,
>>> case SSS_MC_GROUP:
>>> *_offset = offsetof(struct sss_mc_grp_data, strs);
>>> return EOK;
>>>+ case SSS_MC_INITGROUPS:
>>>+ *_offset = offsetof(struct sss_mc_initgr_data, gids);
>>>+ return EOK;
>>> default:
>>> DEBUG(SSSDBG_FATAL_FAILURE, "Unknown memory cache
type.\n");
>>> return EINVAL;
>>>@@ -492,6 +495,9 @@ static errno_t sss_mc_get_strs_len(struct sss_mc_ctx
*mcc,
>>> case SSS_MC_GROUP:
>>> *_len = ((struct sss_mc_grp_data *)&rec->data)->strs_len;
>>> return EOK;
>>>+ case SSS_MC_INITGROUPS:
>>>+ *_len = ((struct sss_mc_initgr_data
*)&rec->data)->data_len;
>>>+ return EOK;
>>> default:
>>> DEBUG(SSSDBG_FATAL_FAILURE, "Unknown memory cache
type.\n");
>>> return EINVAL;
>>>@@ -974,8 +980,8 @@ errno_t sss_mmap_cache_initgr_store(struct sss_mc_ctx
**_mcc,
>>> return EINVAL;
>>> }
>>>
>>>- /* memnum + reserved + array of members + name*/
>>>- data_len = (2 + memnum) * sizeof(uint32_t) + name->len;
>>>+ /* array of members + name */
>>>+ data_len = memnum * sizeof(uint32_t) + name->len;
>>
>>It is actually not array of members but array of group ids
>>the user is member of. It would be good to rename the
>>memnum to num_groups or something similiar.
>>
>>> rec_len = sizeof(struct sss_mc_rec) + sizeof(struct sss_mc_initgr_data)
>>> + data_len;
>>> if (rec_len > mcc->dt_size) {
>>>@@ -998,10 +1004,13 @@ errno_t sss_mmap_cache_initgr_store(struct sss_mc_ctx
**_mcc,
>>> name->str, name->len, name->str,
name->len);
>>>
>>> /* initgroups struct */
>>>+ data->strs_len = name->len;
>>>+ data->data_len = data_len;
>>>+ data->reserved = MC_INVALID_VAL32;
>>> data->members = memnum;
>> ^^^^^^^^^^^^^ ^^^^^^
>>Same as above.
>>
>>> memcpy(data->gids, membuf, memnum * sizeof(uint32_t));
>> ^^^^^^ ^^^^^^
>>gids_buf
>>
>>> memcpy(&data->gids[memnum], name->str, name->len);
>>>- data->name = MC_PTR_DIFF(&data->gids[memnum], data);
>> ^^^^^^
>>>+ data->strs = data->name = MC_PTR_DIFF(&data->gids[memnum],
data);
>>>
>>> MC_LOWER_BARRIER(rec);
>>>
>>>diff --git a/src/sss_client/nss_mc_initgr.c b/src/sss_client/nss_mc_initgr.c
>>>index
bfb09d6550c310fbab254dc9b3ab7b306b7d3f06..0ccde2130526786e1949287cde1e4c1bc4596670 100644
>>>--- a/src/sss_client/nss_mc_initgr.c
>>>+++ b/src/sss_client/nss_mc_initgr.c
>>>@@ -93,6 +93,7 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t
name_len,
>>> uint32_t hash;
>>> uint32_t slot;
>>> int ret;
>>>+ const size_t data_offset = offsetof(struct sss_mc_initgr_data, gids);
>>> uint8_t *max_addr;
>>>
>>> ret = sss_nss_mc_get_ctx("initgroups", &initgr_mc_ctx);
>>>@@ -128,16 +129,21 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name,
size_t name_len,
>>> }
>>>
>>> data = (struct sss_mc_initgr_data *)rec->data;
>>>+ rec_name = (char *)data + data->name;
>>> /* Integrity check
>>>- * - array with gids must be within data_table
>>>- * - string must be within data_table */
>>>- if ((uint8_t *)data->gids > max_addr
>>>- || (uint8_t *)data + data->name + name_len > max_addr)
{
>>>+ * - name_len cannot be longer than all strings or data
>>>+ * - data->name cannot point outside strings
>>>+ * - all data must be within data_table
>>>+ * - name must be within data_table */
>>>+ if (name_len > data->data_len
>>>+ || name_len > data->strs_len
>>>+ || (data->name + name_len) > (data_offset +
data->data_len)
>>>+ || (uint8_t *)data->gids + data->data_len > max_addr
>>>+ || (uint8_t *)rec_name + name_len > max_addr) {
>>> ret = ENOENT;
>>> goto done;
>>> }
>>>
>>>- rec_name = (char *)data + data->name;
>>> if (strcmp(name, rec_name) == 0) {
>>> break;
>>> }
>>>diff --git a/src/util/mmap_cache.h b/src/util/mmap_cache.h
>>>index
438e28a3d217041278fc1bb60aa553d098516035..6cd1e46218fc2a814b34200f0837ee7fff382378 100644
>>>--- a/src/util/mmap_cache.h
>>>+++ b/src/util/mmap_cache.h
>>>@@ -79,7 +79,7 @@ typedef uint32_t rel_ptr_t;
>>>
>>>
>>> #define SSS_MC_MAJOR_VNO 1
>>>-#define SSS_MC_MINOR_VNO 0
>>>+#define SSS_MC_MINOR_VNO 1
>>>
>>> #define SSS_MC_HEADER_UNINIT 0 /* after ftruncate or before reset */
>>> #define SSS_MC_HEADER_ALIVE 1 /* current and in use */
>>>@@ -139,6 +139,10 @@ struct sss_mc_grp_data {
>>>
>>> struct sss_mc_initgr_data {
>>> rel_ptr_t name; /* ptr to name string, rel. to struct base addr
*/
>>>+ rel_ptr_t strs; /* ptr to concatenation of all strings */
>> ^^^^^^^^^^^^^^
>>This pointer is not needed. The name pointer is enough.
>>
>>>+ uint32_t strs_len; /* length of strs */
>> ^^^^^^^^
>>This should be called name_len.
>>
>>>+ uint32_t data_len; /* all initgroups data len */
>>>+ uint32_t reserved;
>>> uint32_t members; /* number of members in groups */
>> ^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>Again not members, number of gids.
>>
>>> uint32_t gids[0]; /* array of all groups
>>> * string with name is stored after gids */
>>>-- 2.4.3
>>
>>
>>If you do not want to squash the renaming into the same patch, feel free
>>to attach it as separate patch.
>Yes, they need to be in separate patch because they are not related
>to the regression.
>
>> I am fine either way. Sorry I should
>>have catch this in the first iteration of the initgr memcache patches.
>>
>I glad the biggest issue in patches was just variable names.
>
>LS
>From e6f0d72167fdd6921e173025a0e374996fc156d5 Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik <lslebodn(a)redhat.com>
>Date: Thu, 16 Jul 2015 16:54:00 +0200
>Subject: [PATCH 1/2] mmap_cache: Rename variables
>
>---
self-NACK
There is a crash in nss responder
Thread 1 (Thread 0x7f43151a8700 (LWP 30847)):
#0 0x00007f431058a174 in __strcmp_sse2 () from /lib64/libc.so.6
No symbol table info available.
#1 0x000000000041768b in sss_mc_find_record (mcc=0x113ee90, key=0x7ffce089a5b0) at
+src/responder/nss/nsssrv_mmap_cache.c:562
rec = 0x7f4306f2b038
hash = 13194
slot = <value optimized out>
name_ptr = <value optimized out>
t_key = <value optimized out>
strs_offset = 24
strs_len = <value optimized out>
max_addr = 0x7f43076cc238 <Address 0x7f43076cc238 out of bounds>
ret = 0
__FUNCTION__ = "sss_mc_find_record"
Attached is a new patch set. I also contains fix for #2712 and not just for
#2716. Otherwise there would be a crash. So it would be better
push them together.
LS