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

["Konstantin Meskhidze (A)" <konstantin.meskhidze@xxxxxxxxxx>]

7/8/2022 7:57 PM, Mickaël Salaün пишет:
> 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.

    Ok. I got it.
> >    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.

 Ok. thanks!!!
> .