Messages in this thread | | | Date | Tue, 30 Aug 2022 13:22:36 +0200 | Subject | Re: [PATCH -next v2 2/6] landlock: abstract walk_to_visible_parent() helper | From | Mickaël Salaün <> |
| |
On 27/08/2022 13:12, Xiu Jianfeng wrote: > This helper will be used in the next commit which supports chmod and > chown access rights restriction. > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > --- > security/landlock/fs.c | 67 ++++++++++++++++++++++++++++++------------ > 1 file changed, 49 insertions(+), 18 deletions(-) > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index c57f581a9cd5..4ef614a4ea22 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -38,6 +38,44 @@ > #include "ruleset.h" > #include "setup.h" > > +enum walk_result { > + WALK_CONTINUE, > + WALK_TO_REAL_ROOT, > + WALK_TO_DISCONN_ROOT,
Why did you created these results instead of the ones I proposed?
> +}; > + > +/* > + * walk to the visible parent, caller should call path_get()/path_put() > + * before/after this helpler. > + * > + * Returns: > + * - WALK_TO_REAL_ROOT if walk to the real root; > + * - WALK_TO_DISCONN_ROOT if walk to disconnected root; > + * - WALK_CONTINUE otherwise. > + */ > +static enum walk_result walk_to_visible_parent(struct path *path) > +{ > + struct dentry *parent_dentry; > +jump_up: > + if (path->dentry == path->mnt->mnt_root) { > + if (follow_up(path)) { > + /* Ignores hidden mount points. */ > + goto jump_up; > + } else { > + /* Stop at the real root. */ > + return WALK_TO_REAL_ROOT; > + } > + } > + /* Stops at disconnected root directories. */ > + if (unlikely(IS_ROOT(path->dentry))) > + return WALK_TO_DISCONN_ROOT; > + parent_dentry = dget_parent(path->dentry); > + dput(path->dentry); > + path->dentry = parent_dentry; > + > + return WALK_CONTINUE; > +} > + > /* Underlying object management */ > > static void release_inode(struct landlock_object *const object) > @@ -539,8 +577,8 @@ static int check_access_path_dual( > * restriction. > */ > while (true) { > - struct dentry *parent_dentry; > const struct landlock_rule *rule; > + enum walk_result wr;
Please make the names understandable. In this case this variable may not be needed anyway.
> > /* > * If at least all accesses allowed on the destination are > @@ -588,20 +626,12 @@ static int check_access_path_dual( > if (allowed_parent1 && allowed_parent2) > break; > > -jump_up: > - if (walker_path.dentry == walker_path.mnt->mnt_root) { > - if (follow_up(&walker_path)) { > - /* Ignores hidden mount points. */ > - goto jump_up; > - } else { > - /* > - * Stops at the real root. Denies access > - * because not all layers have granted access. > - */ > - break; > - } > - } > - if (unlikely(IS_ROOT(walker_path.dentry))) { > + wr = walk_to_visible_parent(&walker_path); > + switch (wr) { > + case WALK_TO_REAL_ROOT: > + /* Stop at the real root. */ > + goto out; > + case WALK_TO_DISCONN_ROOT: > /* > * Stops at disconnected root directories. Only allows > * access to internal filesystems (e.g. nsfs, which is > @@ -609,12 +639,13 @@ static int check_access_path_dual( > */ > allowed_parent1 = allowed_parent2 = > !!(walker_path.mnt->mnt_flags & MNT_INTERNAL);
Why not include this check in the helper? This is then not checked in patch 3 with current_check_access_path_context_only(), which is a bug.
> + goto out; > + case WALK_CONTINUE: > + default: > break; > } > - parent_dentry = dget_parent(walker_path.dentry); > - dput(walker_path.dentry); > - walker_path.dentry = parent_dentry; > } > +out: > path_put(&walker_path); > > if (allowed_parent1 && allowed_parent2)
| |