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

Lukas Slebodnik lslebodn at redhat.com
Thu Jul 16 15:07:40 UTC 2015


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
-------------- next part --------------
>From e6f0d72167fdd6921e173025a0e374996fc156d5 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Thu, 16 Jul 2015 16:54:00 +0200
Subject: [PATCH 1/2] mmap_cache: Rename variables

---
 src/responder/nss/nsssrv_mmap_cache.c | 16 ++++++++--------
 src/responder/nss/nsssrv_mmap_cache.h |  4 ++--
 src/sss_client/nss_mc_initgr.c        | 10 +++++-----
 src/util/mmap_cache.h                 |  2 +-
 4 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index ebda8ac6fab3dd87f5a1d8e43717bf7a5b5a9878..16bc926f3ba4f5ab096bd0fb43895edef2b57c50 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -959,8 +959,8 @@ done:
 
 errno_t sss_mmap_cache_initgr_store(struct sss_mc_ctx **_mcc,
                                     struct sized_string *name,
-                                    uint32_t memnum,
-                                    uint8_t *membuf)
+                                    uint32_t num_groups,
+                                    uint8_t *gids_buf)
 {
     struct sss_mc_ctx *mcc = *_mcc;
     struct sss_mc_rec *rec;
@@ -974,8 +974,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;
+    /* num_groups + reserved + array of gids + name*/
+    data_len = (2 + num_groups) * sizeof(uint32_t) + name->len;
     rec_len = sizeof(struct sss_mc_rec) + sizeof(struct sss_mc_initgr_data)
               + data_len;
     if (rec_len > mcc->dt_size) {
@@ -998,10 +998,10 @@ errno_t sss_mmap_cache_initgr_store(struct sss_mc_ctx **_mcc,
                             name->str, name->len, name->str, name->len);
 
     /* initgroups struct */
-    data->members = memnum;
-    memcpy(data->gids, membuf, memnum * sizeof(uint32_t));
-    memcpy(&data->gids[memnum], name->str, name->len);
-    data->name = MC_PTR_DIFF(&data->gids[memnum], data);
+    data->num_groups = num_groups;
+    memcpy(data->gids, gids_buf, num_groups * sizeof(uint32_t));
+    memcpy(&data->gids[num_groups], name->str, name->len);
+    data->name = MC_PTR_DIFF(&data->gids[num_groups], data);
 
     MC_LOWER_BARRIER(rec);
 
diff --git a/src/responder/nss/nsssrv_mmap_cache.h b/src/responder/nss/nsssrv_mmap_cache.h
index 3a6764dd36bf3346d789bb287b3a94df120f36ee..b09e4a6f8efa364ae7a6407bab9d8a2a2143c812 100644
--- a/src/responder/nss/nsssrv_mmap_cache.h
+++ b/src/responder/nss/nsssrv_mmap_cache.h
@@ -53,8 +53,8 @@ errno_t sss_mmap_cache_gr_store(struct sss_mc_ctx **_mcc,
 
 errno_t sss_mmap_cache_initgr_store(struct sss_mc_ctx **_mcc,
                                     struct sized_string *name,
-                                    uint32_t memnum,
-                                    uint8_t *membuf);
+                                    uint32_t num_groups,
+                                    uint8_t *gids_buf);
 
 errno_t sss_mmap_cache_pw_invalidate(struct sss_mc_ctx *mcc,
                                      struct sized_string *name);
diff --git a/src/sss_client/nss_mc_initgr.c b/src/sss_client/nss_mc_initgr.c
index bfb09d6550c310fbab254dc9b3ab7b306b7d3f06..e21b9f40aba00f9cc2385a561fc2bcc163c5791a 100644
--- a/src/sss_client/nss_mc_initgr.c
+++ b/src/sss_client/nss_mc_initgr.c
@@ -42,7 +42,7 @@ static errno_t sss_nss_mc_parse_result(struct sss_mc_rec *rec,
     struct sss_mc_initgr_data *data;
     time_t expire;
     long int i;
-    uint32_t gid_count;
+    uint32_t num_groups;
     long int max_ret;
 
     /* additional checks before filling result*/
@@ -53,15 +53,15 @@ static errno_t sss_nss_mc_parse_result(struct sss_mc_rec *rec,
     }
 
     data = (struct sss_mc_initgr_data *)rec->data;
-    gid_count = data->members;
-    max_ret = gid_count;
+    num_groups = data->num_groups;
+    max_ret = num_groups;
 
     /* check we have enough space in the buffer */
-    if ((*size - *start) < gid_count) {
+    if ((*size - *start) < num_groups) {
         long int newsize;
         gid_t *newgroups;
 
-        newsize = *size + gid_count;
+        newsize = *size + num_groups;
         if ((limit > 0) && (newsize > limit)) {
             newsize = limit;
             max_ret = newsize - *start;
diff --git a/src/util/mmap_cache.h b/src/util/mmap_cache.h
index 438e28a3d217041278fc1bb60aa553d098516035..a8703d5da4022e11c4bc2f3e2f8710e7f9d982bf 100644
--- a/src/util/mmap_cache.h
+++ b/src/util/mmap_cache.h
@@ -139,7 +139,7 @@ struct sss_mc_grp_data {
 
 struct sss_mc_initgr_data {
     rel_ptr_t name;         /* ptr to name string, rel. to struct base addr */
-    uint32_t members;       /* number of members in groups */
+    uint32_t num_groups;    /* number of groups */
     uint32_t gids[0];       /* array of all groups
                              * string with name is stored after gids */
 };
-- 
2.4.3

-------------- next part --------------
>From 392b9c793882ae356a78bbd92805a4d8f5bd9167 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Thu, 16 Jul 2015 17:00:12 +0200
Subject: [PATCH 2/2] 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 16bc926f3ba4f5ab096bd0fb43895edef2b57c50..ab9e769b1f4d5d17a8c41429afce292298239bc5 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;
     }
 
-    /* num_groups + reserved + array of gids + name*/
-    data_len = (2 + num_groups) * sizeof(uint32_t) + name->len;
+    /* array of gids + name */
+    data_len = num_groups * sizeof(uint32_t) + name->len;
     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->num_groups = num_groups;
     memcpy(data->gids, gids_buf, num_groups * sizeof(uint32_t));
     memcpy(&data->gids[num_groups], name->str, name->len);
-    data->name = MC_PTR_DIFF(&data->gids[num_groups], data);
+    data->strs = data->name = MC_PTR_DIFF(&data->gids[num_groups], data);
 
     MC_LOWER_BARRIER(rec);
 
diff --git a/src/sss_client/nss_mc_initgr.c b/src/sss_client/nss_mc_initgr.c
index e21b9f40aba00f9cc2385a561fc2bcc163c5791a..153617ea9c6489b7439b9676904b42b042f6697c 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->strs + 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 a8703d5da4022e11c4bc2f3e2f8710e7f9d982bf..b5917b3c0973276e43e9fe160cec7528b1224f8f 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 */
+    uint32_t reserved;
+    uint32_t strs_len;      /* length of strs */
+    uint32_t data_len;      /* all initgroups data len */
     uint32_t num_groups;    /* number of groups */
     uint32_t gids[0];       /* array of all groups
                              * string with name is stored after gids */
-- 
2.4.3



More information about the sssd-devel mailing list