This helper will be used in the next commit which supports chmod and
chown access rights restriction.
Signed-off-by: Xiu Jianfeng <[email protected]>
---
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,
+};
+
+/*
+ * 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;
/*
* 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);
+ 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)
--
2.17.1
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 <[email protected]>
> ---
> 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)
Hi,
在 2022/8/30 19:22, 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 <[email protected]>
>> ---
>> 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.
I get your point, after moving it to the helper, here should be:
while (true) {
...
switch(walk_to_visible_parent(&walker_path)) {
case WALK_CONTINUE:
break;
case WALK_ALLOWED:
allowed_parent1 = allowed_parent2 = true;
goto out;
case WR_DENIED:
default:
allowed_parent1 = allowed_parent2 = false;
goto out;
}
}
>
>
>> + 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)
> .