[SSSD] [PATCH] mmap_cache: "Override" functions for initgr mmap cache

Michal Židek mzidek at redhat.com
Thu Jul 16 17:19:50 UTC 2015


On 07/16/2015 05:07 PM, 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 at 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
>
>

I still think we should not use the 'strs' in the sss_mc_initgr_data
structure. If we want to have multiple names in it in the future
it should be called 'names'. The reason is that the we have
getter functions sss_mc_get_strs_offset and sss_mc_get_strs_len
but they have nothing to do with the current 'strs' member in
the sss_mc_initgr_data which is confusing (they work with the
gids here). So either rename the functions to something more generic
or do not use the 'strs' name for the member.

Other than that the patches look good.

Michal

-- 
Senior Principal Intern



More information about the sssd-devel mailing list