Re: [PATCH v6 02/17] landlock: refactors landlock_find/insert_rule

[Mickaël Salaün <mic@xxxxxxxxxxx>]

On 08/07/2022 16:20, Konstantin Meskhidze (A) wrote:
> 7/8/2022 4:56 PM, Mickaël Salaün пишет:
> > On 08/07/2022 14:53, Konstantin Meskhidze (A) wrote:
> > > 7/7/2022 7:44 PM, Mickaël Salaün пишет:
> > > > On 21/06/2022 10:22, Konstantin Meskhidze wrote:
> > > > > Adds a new object union to support a socket port
> > > > > rule type. Refactors landlock_insert_rule() and
> > > > > landlock_find_rule() to support coming network
> > > > > modifications. Now adding or searching a rule
> > > > > in a ruleset depends on a rule_type argument
> > > > > provided in refactored functions mentioned above.
> > > > >
> > > > > Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Changes since v5:
> > > > > * Formats code with clang-format-14.
> > > > >
> > > > > Changes since v4:
> > > > > * Refactors insert_rule() and create_rule() functions by deleting
> > > > > rule_type from their arguments list, it helps to reduce useless code.
> > > > >
> > > > > Changes since v3:
> > > > > * Splits commit.
> > > > > * Refactors landlock_insert_rule and landlock_find_rule functions.
> > > > > * Rename new_ruleset->root_inode.
> > > > >
> > > > > ---
> > > > >   security/landlock/fs.c      |   7 ++-
> > > > >   security/landlock/ruleset.c | 105 
> > > > > ++++++++++++++++++++++++++----------
> > > > >   security/landlock/ruleset.h |  27 +++++-----
> > > > >   3 files changed, 96 insertions(+), 43 deletions(-)
> > > > >
> > > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > > > > index e6da08ed99d1..46aedc2a05a8 100644
> > > > > --- a/security/landlock/fs.c
> > > > > +++ b/security/landlock/fs.c
> > > > > @@ -173,7 +173,8 @@ int landlock_append_fs_rule(struct 
> > > > > landlock_ruleset *const ruleset,
> > > > >       if (IS_ERR(object))
> > > > >           return PTR_ERR(object);
> > > > >       mutex_lock(&ruleset->lock);
> > > > > -    err = landlock_insert_rule(ruleset, object, access_rights);
> > > > > +    err = landlock_insert_rule(ruleset, object, 0, access_rights,
> > > > > +                   LANDLOCK_RULE_PATH_BENEATH);
> > > > >       mutex_unlock(&ruleset->lock);
> > > > >       /*
> > > > >        * No need to check for an error because landlock_insert_rule()
> > > > > @@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const 
> > > > > domain,
> > > > >       inode = d_backing_inode(dentry);
> > > > >       rcu_read_lock();
> > > > >       rule = landlock_find_rule(
> > > > > -        domain, rcu_dereference(landlock_inode(inode)->object));
> > > > > +        domain,
> > > > > +        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
> > > > > +        LANDLOCK_RULE_PATH_BENEATH);
> > > > >       rcu_read_unlock();
> > > > >       return rule;
> > > > >   }
> > > > > diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> > > > > index a3fd58d01f09..5f13f8a12aee 100644
> > > > > --- a/security/landlock/ruleset.c
> > > > > +++ b/security/landlock/ruleset.c
> > > > > @@ -35,7 +35,7 @@ static struct landlock_ruleset 
> > > > > *create_ruleset(const u32 num_layers)
> > > > >           return ERR_PTR(-ENOMEM);
> > > > >       refcount_set(&new_ruleset->usage, 1);
> > > > >       mutex_init(&new_ruleset->lock);
> > > > > -    new_ruleset->root = RB_ROOT;
> > > > > +    new_ruleset->root_inode = RB_ROOT;
> > > > >       new_ruleset->num_layers = num_layers;
> > > > >       /*
> > > > >        * hierarchy = NULL
> > > > > @@ -69,7 +69,8 @@ static void build_check_rule(void)
> > > > >   }
> > > > >
> > > > >   static struct landlock_rule *
> > > > > -create_rule(struct landlock_object *const object,
> > > > > +create_rule(struct landlock_object *const object_ptr,
> > > > > +        const uintptr_t object_data,
> > > > >           const struct landlock_layer (*const layers)[], const u32 
> > > > > num_layers,
> > > > >           const struct landlock_layer *const new_layer)
> > > > >   {
> > > > > @@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
> > > > >       if (!new_rule)
> > > > >           return ERR_PTR(-ENOMEM);
> > > > >       RB_CLEAR_NODE(&new_rule->node);
> > > > > -    landlock_get_object(object);
> > > > > -    new_rule->object = object;
> > > > > +
> > > > > +    if (object_ptr) {
> > > > > +        landlock_get_object(object_ptr);
> > > > > +        new_rule->object.ptr = object_ptr;
> > > > > +    } else if (object_ptr && object_data) {
> > > >
> > > > Something is wrong with this second check: else + object_ptr?
> > >
> > > It was your suggestion to use it like this:
> > > " ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."
> > >
> > > Please check it here:
> > > https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@xxxxxxxxxxx/ 
> >
> >
> > Yes, but the error is in the "else", you should write:
> > if (WARN_ON_ONCE(object_ptr && object_data))
> >     return ERR_PTR(-EINVAL);
> >
> > …and this should be before the `if (object_ptr) {` line (to avoid
> > erronous landlock_get_object() call), just after the `if (!new_rule)` 
> > check.
>
>    Maybe we could delete this check here cause we have it in the upper 
> insert_rule() function??
>
> ...
>      if (WARN_ON_ONCE(!layers))
>          return -ENOENT;
> ------>    if (WARN_ON_ONCE(object_ptr && object_data))
>          return -EINVAL;
>      /* Chooses rb_tree structure depending on a rule type. */
>      switch (rule_type) {
>      case LANDLOCK_RULE_PATH_BENEATH:
>          if (WARN_ON_ONCE(!object_ptr))
>              return -ENOENT;
>          object_data = (uintptr_t)object_ptr;
>          root = &ruleset->root_inode;
>          break;
>      default:
>          WARN_ON_ONCE(1);
>          return -EINVAL;
>      }
> ...
>
> This is double check here. What do you think?

This check is indeed done twice, and for now create_rule() is only 
called from insert_rule(), but I prefer to keep it in both location to 
not get bitten in the future were it could be called from other 
locations. The compiler may be smart enough to remove the redundant 
checks though.

I'll send a patch to improve this part.