Re: [PATCH] Restorecon: factor out a lookup helper for context matches

[Stephen Smalley <sds@tycho.nsa.gov>]

On 7/23/19 4:06 PM, Nicolas Iooss wrote:
> On Wed, Jun 19, 2019 at 4:45 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>> On 3/11/19 6:24 PM, xunchang wrote:
>>> We used to hash the file_context and skip the restorecon on the top
>>> level directory if the hash doesn't change. But the file_context might
>>> change after an update; and some users experienced long restorecon
>>> time as they have lots of files under directories like /data/media.
>>> Therefore, we try to skip unnecessary restores if the file context
>>> relates to the given directory doesn't change.
>>>
>>> This CL is the first step that factors out a lookup helper function
>>> and returns an array of matched pointers instead of a single one.
>>> The old loopup_common function is then modified to take the first
>>> element in the array.
>>>
>>> This change has already been submitted in android selinux branch. And
>>> porting it upstream will make these two branches more consistent and
>>> save some work for the future merges.
>>
>> There were some changes to this patch before it landed in AOSP, so they
>> aren't quite consistent.  Do you want to submit the final patch?
> 
> Hello,
> 
> What are the states of this patch and the one which has been posted in
> April (https://lore.kernel.org/selinux/20190417180955.136942-1-xunchang@google.com/)?
> I do not follow what happens in Android but if the patches have been
> modified there, it seems a good idea to post the modified patches to
> selinux@vger.kernel.org.

FWIW, the Android patch that was ultimately merged can be seen and 
downloaded from 
https://android-review.googlesource.com/c/platform/external/selinux/+/918713. 
  The posted patch was the version 1 patch.


> 
> Thanks,
> Nicolas
> 
>>>
>>> Signed-off-by: Tianjie Xu <xunchang@google.com>
>>> ---
>>>    libselinux/include/selinux/label.h |   4 ++
>>>    libselinux/src/label.c             |   9 +++
>>>    libselinux/src/label_file.c        | 111 +++++++++++++++++++++++------
>>>    libselinux/src/label_internal.h    |   2 +
>>>    4 files changed, 106 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/libselinux/include/selinux/label.h b/libselinux/include/selinux/label.h
>>> index 277287ed..e537aa11 100644
>>> --- a/libselinux/include/selinux/label.h
>>> +++ b/libselinux/include/selinux/label.h
>>> @@ -7,6 +7,7 @@
>>>    #define _SELABEL_H_
>>>
>>>    #include <stdbool.h>
>>> +#include <stdint.h>
>>>    #include <sys/types.h>
>>>    #include <selinux/selinux.h>
>>>
>>> @@ -105,6 +106,9 @@ int selabel_lookup_raw(struct selabel_handle *handle, char **con,
>>>
>>>    bool selabel_partial_match(struct selabel_handle *handle, const char *key);
>>>
>>> +bool selabel_hash_all_partial_matches(struct selabel_handle *rec,
>>> +                                      const char *key, uint8_t* digest);
>>> +
>>>    int selabel_lookup_best_match(struct selabel_handle *rec, char **con,
>>>                              const char *key, const char **aliases, int type);
>>>    int selabel_lookup_best_match_raw(struct selabel_handle *rec, char **con,
>>> diff --git a/libselinux/src/label.c b/libselinux/src/label.c
>>> index 8d586bda..1d16f685 100644
>>> --- a/libselinux/src/label.c
>>> +++ b/libselinux/src/label.c
>>> @@ -274,6 +274,15 @@ bool selabel_partial_match(struct selabel_handle *rec, const char *key)
>>>        return rec->func_partial_match(rec, key);
>>>    }
>>>
>>> +bool selabel_hash_all_partial_matches(struct selabel_handle *rec,
>>> +                                      const char *key, uint8_t *digest) {
>>> +     if (!rec->func_hash_all_partial_matches) {
>>> +             return false;
>>> +     }
>>> +
>>> +     return rec->func_hash_all_partial_matches(rec, key, digest);
>>> +}
>>> +
>>>    int selabel_lookup_best_match(struct selabel_handle *rec, char **con,
>>>                              const char *key, const char **aliases, int type)
>>>    {
>>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>>> index b81fd552..90cbd666 100644
>>> --- a/libselinux/src/label_file.c
>>> +++ b/libselinux/src/label_file.c
>>> @@ -843,22 +843,38 @@ static void closef(struct selabel_handle *rec)
>>>        free(data);
>>>    }
>>>
>>> -static struct spec *lookup_common(struct selabel_handle *rec,
>>> -                                          const char *key,
>>> -                                          int type,
>>> -                                          bool partial)
>>> +// Finds all the matches of |key| in the given context. Returns the result in
>>> +// the allocated array and updates the match count. If match_count is NULL,
>>> +// stops early once the 1st match is found.
>>> +static const struct spec **lookup_all(struct selabel_handle *rec,
>>> +                                      const char *key,
>>> +                                      int type,
>>> +                                      bool partial,
>>> +                                      size_t *match_count)
>>>    {
>>>        struct saved_data *data = (struct saved_data *)rec->data;
>>>        struct spec *spec_arr = data->spec_arr;
>>>        int i, rc, file_stem;
>>>        mode_t mode = (mode_t)type;
>>>        const char *buf;
>>> -     struct spec *ret = NULL;
>>>        char *clean_key = NULL;
>>>        const char *prev_slash, *next_slash;
>>>        unsigned int sofar = 0;
>>>        char *sub = NULL;
>>>
>>> +     const struct spec **result = NULL;
>>> +     if (match_count) {
>>> +             *match_count = 0;
>>> +             result = calloc(data->nspec, sizeof(struct spec*));
>>> +     } else {
>>> +             result = calloc(1, sizeof(struct spec*));
>>> +     }
>>> +     if (!result) {
>>> +             selinux_log(SELINUX_ERROR, "Failed to allocate %zu bytes of data\n",
>>> +                         data->nspec * sizeof(struct spec*));
>>> +             goto finish;
>>> +     }
>>> +
>>>        if (!data->nspec) {
>>>                errno = ENOENT;
>>>                goto finish;
>>> @@ -899,18 +915,33 @@ static struct spec *lookup_common(struct selabel_handle *rec,
>>>                 * specified or if the mode matches the file mode then we do
>>>                 * a regex check        */
>>>                if ((spec->stem_id == -1 || spec->stem_id == file_stem) &&
>>> -                 (!mode || !spec->mode || mode == spec->mode)) {
>>> +                             (!mode || !spec->mode || mode == spec->mode)) {
>>>                        if (compile_regex(data, spec, NULL) < 0)
>>>                                goto finish;
>>>                        if (spec->stem_id == -1)
>>>                                rc = regex_match(spec->regex, key, partial);
>>>                        else
>>>                                rc = regex_match(spec->regex, buf, partial);
>>> -                     if (rc == REGEX_MATCH) {
>>> -                             spec->matches++;
>>> -                             break;
>>> -                     } else if (partial && rc == REGEX_MATCH_PARTIAL)
>>> +
>>> +                     if (rc == REGEX_MATCH || (partial && rc == REGEX_MATCH_PARTIAL)) {
>>> +                             if (rc == REGEX_MATCH) {
>>> +                                     spec->matches++;
>>> +                             }
>>> +
>>> +                             if (strcmp(spec_arr[i].lr.ctx_raw, "<<none>>") == 0) {
>>> +                                     errno = ENOENT;
>>> +                                     goto finish;
>>> +                             }
>>> +
>>> +                             if (match_count) {
>>> +                                     result[*match_count] = spec;
>>> +                                     *match_count += 1;
>>> +                                     // Continue to find all the matches.
>>> +                                     continue;
>>> +                             }
>>> +                             result[0] = spec;
>>>                                break;
>>> +                     }
>>>
>>>                        if (rc == REGEX_NO_MATCH)
>>>                                continue;
>>> @@ -921,19 +952,58 @@ static struct spec *lookup_common(struct selabel_handle *rec,
>>>                }
>>>        }
>>>
>>> -     if (i < 0 || strcmp(spec_arr[i].lr.ctx_raw, "<<none>>") == 0) {
>>> -             /* No matching specification. */
>>> -             errno = ENOENT;
>>> -             goto finish;
>>> -     }
>>> -
>>> -     errno = 0;
>>> -     ret = &spec_arr[i];
>>> -
>>>    finish:
>>>        free(clean_key);
>>>        free(sub);
>>> -     return ret;
>>> +     if (result && !result[0]) {
>>> +             free(result);
>>> +             result = NULL;
>>> +     }
>>> +     return result;
>>> +}
>>> +
>>> +static struct spec *lookup_common(struct selabel_handle *rec,
>>> +                                  const char *key,
>>> +                                  int type,
>>> +                                  bool partial) {
>>> +     const struct spec **matches = lookup_all(rec, key, type, partial, NULL);
>>> +     if (!matches) {
>>> +             return NULL;
>>> +     }
>>> +     struct spec *result = (struct spec*)matches[0];
>>> +     free(matches);
>>> +     return result;
>>> +}
>>> +
>>> +static bool hash_all_partial_matches(struct selabel_handle *rec, const char *key, uint8_t *digest)
>>> +{
>>> +     assert(digest);
>>> +
>>> +     size_t total_matches;
>>> +     const struct spec **matches = lookup_all(rec, key, 0, true, &total_matches);
>>> +     if (!matches) {
>>> +             return false;
>>> +     }
>>> +
>>> +     Sha1Context context;
>>> +     Sha1Initialise(&context);
>>> +     size_t i;
>>> +     for (i = 0; i < total_matches; i++) {
>>> +             char* regex_str = matches[i]->regex_str;
>>> +             mode_t mode = matches[i]->mode;
>>> +             char* ctx_raw = matches[i]->lr.ctx_raw;
>>> +
>>> +             Sha1Update(&context, regex_str, strlen(regex_str) + 1);
>>> +             Sha1Update(&context, &mode, sizeof(mode_t));
>>> +             Sha1Update(&context, ctx_raw, strlen(ctx_raw) + 1);
>>> +     }
>>> +
>>> +     SHA1_HASH sha1_hash;
>>> +     Sha1Finalise(&context, &sha1_hash);
>>> +     memcpy(digest, sha1_hash.bytes, SHA1_HASH_SIZE);
>>> +
>>> +     free(matches);
>>> +     return true;
>>>    }
>>>
>>>    static struct selabel_lookup_rec *lookup(struct selabel_handle *rec,
>>> @@ -1133,6 +1203,7 @@ int selabel_file_init(struct selabel_handle *rec,
>>>        rec->func_stats = &stats;
>>>        rec->func_lookup = &lookup;
>>>        rec->func_partial_match = &partial_match;
>>> +     rec->func_hash_all_partial_matches = &hash_all_partial_matches;
>>>        rec->func_lookup_best_match = &lookup_best_match;
>>>        rec->func_cmp = &cmp;
>>>
>>> diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h
>>> index 0e020557..1fa5ade6 100644
>>> --- a/libselinux/src/label_internal.h
>>> +++ b/libselinux/src/label_internal.h
>>> @@ -87,6 +87,8 @@ struct selabel_handle {
>>>        void (*func_close) (struct selabel_handle *h);
>>>        void (*func_stats) (struct selabel_handle *h);
>>>        bool (*func_partial_match) (struct selabel_handle *h, const char *key);
>>> +     bool (*func_hash_all_partial_matches) (struct selabel_handle *h,
>>> +                                            const char *key, uint8_t *digest);
>>>        struct selabel_lookup_rec *(*func_lookup_best_match)
>>>                                                    (struct selabel_handle *h,
>>>                                                    const char *key,
>>>
>>
>