Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1232329 > unrolled thread

Re: [RFC v7 26/41] richacl: Apply the file masks to a richacl

Started bybfields@fieldses.org (J. Bruce Fields)
First post2015-09-24 20:40 +0200
Last post2015-09-25 20:40 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [RFC v7 26/41] richacl: Apply the file masks to a richacl bfields@fieldses.org (J. Bruce Fields) - 2015-09-24 20:40 +0200
    Re: [PATCH] richacl: Possible other write-through fix Andreas Gruenbacher <agruenba@redhat.com> - 2015-09-25 18:40 +0200
    Re: [PATCH] richacl: Possible other write-through fix Andreas Gruenbacher <agruenba@redhat.com> - 2015-09-25 18:50 +0200
      Re: [PATCH] richacl: Possible other write-through fix "J. Bruce Fields" <bfields@fieldses.org> - 2015-09-25 20:40 +0200

#1232329 — Re: [RFC v7 26/41] richacl: Apply the file masks to a richacl

Frombfields@fieldses.org (J. Bruce Fields)
Date2015-09-24 20:40 +0200
SubjectRe: [RFC v7 26/41] richacl: Apply the file masks to a richacl
Message-ID<qcjB0-2LT-17@gated-at.bofh.it>
On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
> Put all the pieces of the acl transformation puzzle together for
> computing a richacl which has the file masks "applied" so that the
> standard nfsv4 access check algorithm can be used on the richacl.
> 
> Signed-off-by: Andreas Gruenbacher <agruen@kernel.org>
> ---
>  fs/richacl_compat.c     | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/richacl.h |   3 ++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 412844c..9681efe 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -730,3 +730,113 @@ richacl_isolate_group_class(struct richacl_alloc *alloc)
>  	}
>  	return 0;
>  }
> +
> +/**
> + * __richacl_apply_masks  -  apply the file masks to all aces
> + * @alloc:	acl and number of allocated entries
> + *
> + * Apply the owner mask to owner@ aces, the other mask to
> + * everyone@ aces, and the group mask to all other aces.
> + *
> + * The previous transformations have brought the acl into a
> + * form in which applying the masks will not lead to the
> + * accidental loss of permissions anymore.
> + */
> +static int
> +__richacl_apply_masks(struct richacl_alloc *alloc, kuid_t owner)
> +{
> +	struct richace *ace;
> +
> +	richacl_for_each_entry(ace, alloc->acl) {
> +		unsigned int mask;
> +
> +		if (richace_is_inherit_only(ace) || !richace_is_allow(ace))
> +			continue;
> +		if (richace_is_owner(ace) ||
> +		    (richace_is_unix_user(ace) && uid_eq(owner, ace->e_id.uid)))
> +			mask = alloc->acl->a_owner_mask;
> +		else if (richace_is_everyone(ace))
> +			mask = alloc->acl->a_other_mask;
> +		else
> +			mask = alloc->acl->a_group_mask;
> +		if (richace_change_mask(alloc, &ace, ace->e_mask & mask))
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * richacl_apply_masks  -  apply the masks to the acl
> + *
> + * Transform @acl so that the standard NFSv4 permission check algorithm (which
> + * is not aware of file masks) will compute the same access decisions as the
> + * richacl permission check algorithm (which looks at the acl and the file
> + * masks).
> + *
> + * This algorithm is split into several steps:
> + *
> + *   - Move everyone@ aces to the end of the acl.  This simplifies the other
> + *     transformations, and allows the everyone@ allow ace at the end of the
> + *     acl to eventually allow permissions to the other class only.
> + *
> + *   - Propagate everyone@ permissions up the acl.  This transformation makes
> + *     sure that the owner and group class aces won't lose any permissions when
> + *     we apply the other mask to the everyone@ allow ace at the end of the acl.
> + *
> + *   - Apply the file masks to all aces.
> + *
> + *   - Make sure the owner is granted the owner mask permissions.
> + *
> + *   - Make sure everyone is granted the other mask permissions.
> + *
> + *   - Make sure that the owner is not granted any permissions beyond the owner
> + *     mask from group class aces or from everyone@.
> + *
> + *   - Make sure that the group class is not granted any permissions from
> + *     everyone@.
> + *
> + * The algorithm is exact except for richacls which cannot be represented as an
> + * acl alone: for example, given this acl:
> + *
> + *    group@:rw::allow
> + *
> + * when file masks corresponding to mode 0600 are applied, the owner would only
> + * get rw access if he is a member of the owning group.  This algorithm would
> + * produce an empty acl in this case.  We fix this case by modifying
> + * richacl_permission() so that the group mask is always applied to group class
> + * aces.  With this fix, the owner would not have any access (beyond the
> + * implicit permissions always granted to owners).
> + *
> + * NOTE: Depending on the acl and file masks, this algorithm can increase the
> + * number of aces by almost a factor of three in the worst case. This may make
> + * the acl too large for some purposes.
> + */
> +int
> +richacl_apply_masks(struct richacl **acl, kuid_t owner)
> +{
> +	if ((*acl)->a_flags & RICHACL_MASKED) {
> +		struct richacl_alloc alloc = {
> +			.acl = richacl_clone(*acl, GFP_KERNEL),
> +			.count = (*acl)->a_count,
> +		};
> +		if (!alloc.acl)
> +			return -ENOMEM;
> +
> +		if (richacl_move_everyone_aces_down(&alloc) ||
> +		    richacl_propagate_everyone(&alloc) ||
> +		    __richacl_apply_masks(&alloc, owner) ||
> +		    richacl_set_owner_permissions(&alloc) ||
> +		    richacl_set_other_permissions(&alloc) ||

Hm, I'm a little unsure about this step: it seems like the one step that
can actually change the permissions (making them more permissive, in
this case), whereas the others are neutral.

The following two steps should fix that, but I'm not sure they do.

E.g. if I have an ACL like

	mask 0777, WRITE_THROUGH
	GROUP@:r--::allow

I think it should result in

	OWNER@:   rwx::allow
	GROUP@:   -wx::deny
	GROUP@:   r--::allow
	EVERYONE@:rwx::allow

But does the GROUP@ deny actually get in there?  It looks to me like the
denies inserted by richacl_isolate_group_class only take into account
the group mask, not the actual permissions granted to any group class
user.

I may be mising something.

--b.

> +		    richacl_isolate_owner_class(&alloc) ||
> +		    richacl_isolate_group_class(&alloc)) {
> +			richacl_put(alloc.acl);
> +			return -ENOMEM;
> +		}
> +
> +		alloc.acl->a_flags &= ~(RICHACL_WRITE_THROUGH | RICHACL_MASKED);
> +		richacl_put(*acl);
> +		*acl = alloc.acl;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(richacl_apply_masks);
> diff --git a/include/linux/richacl.h b/include/linux/richacl.h
> index 832b06c..a945f3c 100644
> --- a/include/linux/richacl.h
> +++ b/include/linux/richacl.h
> @@ -332,4 +332,7 @@ extern struct richacl *richacl_inherit(const struct richacl *, int);
>  extern int richacl_permission(struct inode *, const struct richacl *, int);
>  extern struct richacl *richacl_create(struct inode *, struct inode *);
>  
> +/* richacl_compat.c */
> +extern int richacl_apply_masks(struct richacl **, kuid_t);
> +
>  #endif /* __RICHACL_H */
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]


#1232931 — Re: [PATCH] richacl: Possible other write-through fix

FromAndreas Gruenbacher <agruenba@redhat.com>
Date2015-09-25 18:40 +0200
SubjectRe: [PATCH] richacl: Possible other write-through fix
Message-ID<qcEcq-6SE-21@gated-at.bofh.it>
In reply to#1232329
2015-09-24 20:33 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
>> +int
>> +richacl_apply_masks(struct richacl **acl, kuid_t owner)
>> +{
>> +     if ((*acl)->a_flags & RICHACL_MASKED) {
>> +             struct richacl_alloc alloc = {
>> +                     .acl = richacl_clone(*acl, GFP_KERNEL),
>> +                     .count = (*acl)->a_count,
>> +             };
>> +             if (!alloc.acl)
>> +                     return -ENOMEM;
>> +
>> +             if (richacl_move_everyone_aces_down(&alloc) ||
>> +                 richacl_propagate_everyone(&alloc) ||
>> +                 __richacl_apply_masks(&alloc, owner) ||
>> +                 richacl_set_owner_permissions(&alloc) ||
>> +                 richacl_set_other_permissions(&alloc) ||
>
> Hm, I'm a little unsure about this step: it seems like the one step that
> can actually change the permissions (making them more permissive, in
> this case), whereas the others are neutral.
>
> The following two steps should fix that, but I'm not sure they do.
>
> E.g. if I have an ACL like
>
>         mask 0777, WRITE_THROUGH
>         GROUP@:r--::allow
>
> I think it should result in
>
>         OWNER@:   rwx::allow
>         GROUP@:   -wx::deny
>         GROUP@:   r--::allow
>         EVERYONE@:rwx::allow
>
> But does the GROUP@ deny actually get in there?  It looks to me like the
> denies inserted by richacl_isolate_group_class only take into account
> the group mask, not the actual permissions granted to any group class
> user.
>
> I may be mising something.

Thanks for the test case and analysis. The group@ deny ace that should be
inserted here actually doesn't get inserted. I'm attaching a fix.

---

By the way, all those scenarios can easily be tries out using the test
suite in the user-space richacl package, even without a richacl enabled
kernel. In this case, without the fix, we get:

  $ make tests/richacl-apply-masks
  $ tests/richacl-apply-masks -m 777 group@:r::allow
  group@:r::allow
  everyone@:rwpx::allow

With the fix, we are now getting:

  $ tests/richacl-apply-masks -m 777 group@:r::allow
  owner@:rwpx::allow
  group@:r::allow
  group@:wpx::deny
  everyone@:rwpx::allow

Thanks,
Andreas

diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
index 726d796..bc0bcfe 100644
--- a/fs/richacl_compat.c
+++ b/fs/richacl_compat.c
@@ -463,23 +463,28 @@ richacl_set_owner_permissions(struct richacl_alloc *alloc)
 
 /**
  * richacl_set_other_permissions  -  set the other permissions to the other mask
+ * @alloc:	acl and number of allocated entries
+ * @added:	permissions added for everyone@
  *
  * Change the acl so that everyone@ is granted the permissions set in the other
  * mask.  This leaves at most one efective everyone@ allow entry at the end of
- * the acl.
+ * the acl.  If everyone@ end up being granted additional permissions, these
+ * permissions are returned in @added.
  */
 static int
-richacl_set_other_permissions(struct richacl_alloc *alloc)
+richacl_set_other_permissions(struct richacl_alloc *alloc, unsigned int *added)
 {
 	struct richacl *acl = alloc->acl;
 	unsigned int x = RICHACE_POSIX_ALWAYS_ALLOWED;
 	unsigned int other_mask = acl->a_other_mask & ~x;
-	struct richace *ace = acl->a_entries + acl->a_count - 1;
+	struct richace *ace;
 
 	if (!(other_mask &&
 	      (acl->a_flags & RICHACL_WRITE_THROUGH)))
 		return 0;
 
+	*added = other_mask;
+	ace = acl->a_entries + acl->a_count - 1;
 	if (acl->a_count == 0 ||
 	    !richace_is_everyone(ace) ||
 	    richace_is_inherit_only(ace)) {
@@ -490,8 +495,10 @@ richacl_set_other_permissions(struct richacl_alloc *alloc)
 		ace->e_flags = RICHACE_SPECIAL_WHO;
 		ace->e_mask = other_mask;
 		ace->e_id.special = RICHACE_EVERYONE_SPECIAL_ID;
-	} else
+	} else {
+		*added &= ~ace->e_mask;
 		richace_change_mask(alloc, &ace, other_mask);
+	}
 	return 0;
 }
 
@@ -645,6 +652,7 @@ __richacl_isolate_who(struct richacl_alloc *alloc, struct richace *who,
 /**
  * richacl_isolate_group_class  -  limit the group class to the group file mask
  * @alloc:	acl and number of allocated entries
+ * @deny:	additional permissions to deny
  *
  * POSIX requires that after a chmod, the group class is granted no more
  * permissions than the group file permission bits.  For richacls, this
@@ -679,21 +687,20 @@ __richacl_isolate_who(struct richacl_alloc *alloc, struct richace *who,
  *    everyone@:rw::allow
  */
 static int
-richacl_isolate_group_class(struct richacl_alloc *alloc)
+richacl_isolate_group_class(struct richacl_alloc *alloc, unsigned int deny)
 {
 	struct richace who = {
 		.e_flags = RICHACE_SPECIAL_WHO,
 		.e_id.special = RICHACE_GROUP_SPECIAL_ID,
 	};
 	struct richace *ace;
-	unsigned int deny;
 
 	if (!alloc->acl->a_count)
 		return 0;
 	ace = alloc->acl->a_entries + alloc->acl->a_count - 1;
 	if (richace_is_inherit_only(ace) || !richace_is_everyone(ace))
 		return 0;
-	deny = ace->e_mask & ~alloc->acl->a_group_mask;
+	deny |= ace->e_mask & ~alloc->acl->a_group_mask;
 
 	if (deny) {
 		unsigned int n;
@@ -806,16 +813,17 @@ richacl_apply_masks(struct richacl **acl, kuid_t owner)
 			.acl = richacl_clone(*acl, GFP_KERNEL),
 			.count = (*acl)->a_count,
 		};
+		unsigned int added = 0;
+
 		if (!alloc.acl)
 			return -ENOMEM;
-
 		if (richacl_move_everyone_aces_down(&alloc) ||
 		    richacl_propagate_everyone(&alloc) ||
 		    __richacl_apply_masks(&alloc, owner) ||
+		    richacl_set_other_permissions(&alloc, &added) ||
+		    richacl_isolate_group_class(&alloc, added) ||
 		    richacl_set_owner_permissions(&alloc) ||
-		    richacl_set_other_permissions(&alloc) ||
-		    richacl_isolate_owner_class(&alloc) ||
-		    richacl_isolate_group_class(&alloc)) {
+		    richacl_isolate_owner_class(&alloc)) {
 			richacl_put(alloc.acl);
 			return -ENOMEM;
 		}
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1232935 — Re: [PATCH] richacl: Possible other write-through fix

FromAndreas Gruenbacher <agruenba@redhat.com>
Date2015-09-25 18:50 +0200
SubjectRe: [PATCH] richacl: Possible other write-through fix
Message-ID<qcEm5-746-1@gated-at.bofh.it>
In reply to#1232329
2015-09-24 20:33 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
>> +int
>> +richacl_apply_masks(struct richacl **acl, kuid_t owner)
>> +{
>> +     if ((*acl)->a_flags & RICHACL_MASKED) {
>> +             struct richacl_alloc alloc = {
>> +                     .acl = richacl_clone(*acl, GFP_KERNEL),
>> +                     .count = (*acl)->a_count,
>> +             };
>> +             if (!alloc.acl)
>> +                     return -ENOMEM;
>> +
>> +             if (richacl_move_everyone_aces_down(&alloc) ||
>> +                 richacl_propagate_everyone(&alloc) ||
>> +                 __richacl_apply_masks(&alloc, owner) ||
>> +                 richacl_set_owner_permissions(&alloc) ||
>> +                 richacl_set_other_permissions(&alloc) ||
>
> Hm, I'm a little unsure about this step: it seems like the one step that
> can actually change the permissions (making them more permissive, in
> this case), whereas the others are neutral.
>
> The following two steps should fix that, but I'm not sure they do.
>
> E.g. if I have an ACL like
>
>         mask 0777, WRITE_THROUGH
>         GROUP@:r--::allow
>
> I think it should result in
>
>         OWNER@:   rwx::allow
>         GROUP@:   -wx::deny
>         GROUP@:   r--::allow
>         EVERYONE@:rwx::allow
>
> But does the GROUP@ deny actually get in there?  It looks to me like the
> denies inserted by richacl_isolate_group_class only take into account
> the group mask, not the actual permissions granted to any group class
> user.
>
> I may be mising something.

Thanks for the test case and analysis. The group@ deny ace that should be
inserted here actually doesn't get inserted. I'm attaching a fix.

---

By the way, all those scenarios can easily be tries out using the test
suite in the user-space richacl package, even without a richacl enabled
kernel. In this case, without the fix, we get:

  $ make tests/richacl-apply-masks
  $ tests/richacl-apply-masks -m 777 group@:r::allow
  group@:r::allow
  everyone@:rwpx::allow

With the fix, we are now getting:

  $ tests/richacl-apply-masks -m 777 group@:r::allow
  owner@:rwpx::allow
  group@:r::allow
  group@:wpx::deny
  everyone@:rwpx::allow

Thanks,
Andreas

diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
index 726d796..bc0bcfe 100644
--- a/fs/richacl_compat.c
+++ b/fs/richacl_compat.c
@@ -463,23 +463,28 @@ richacl_set_owner_permissions(struct richacl_alloc *alloc)
 
 /**
  * richacl_set_other_permissions  -  set the other permissions to the other mask
+ * @alloc:	acl and number of allocated entries
+ * @added:	permissions added for everyone@
  *
  * Change the acl so that everyone@ is granted the permissions set in the other
  * mask.  This leaves at most one efective everyone@ allow entry at the end of
- * the acl.
+ * the acl.  If everyone@ end up being granted additional permissions, these
+ * permissions are returned in @added.
  */
 static int
-richacl_set_other_permissions(struct richacl_alloc *alloc)
+richacl_set_other_permissions(struct richacl_alloc *alloc, unsigned int *added)
 {
 	struct richacl *acl = alloc->acl;
 	unsigned int x = RICHACE_POSIX_ALWAYS_ALLOWED;
 	unsigned int other_mask = acl->a_other_mask & ~x;
-	struct richace *ace = acl->a_entries + acl->a_count - 1;
+	struct richace *ace;
 
 	if (!(other_mask &&
 	      (acl->a_flags & RICHACL_WRITE_THROUGH)))
 		return 0;
 
+	*added = other_mask;
+	ace = acl->a_entries + acl->a_count - 1;
 	if (acl->a_count == 0 ||
 	    !richace_is_everyone(ace) ||
 	    richace_is_inherit_only(ace)) {
@@ -490,8 +495,10 @@ richacl_set_other_permissions(struct richacl_alloc *alloc)
 		ace->e_flags = RICHACE_SPECIAL_WHO;
 		ace->e_mask = other_mask;
 		ace->e_id.special = RICHACE_EVERYONE_SPECIAL_ID;
-	} else
+	} else {
+		*added &= ~ace->e_mask;
 		richace_change_mask(alloc, &ace, other_mask);
+	}
 	return 0;
 }
 
@@ -645,6 +652,7 @@ __richacl_isolate_who(struct richacl_alloc *alloc, struct richace *who,
 /**
  * richacl_isolate_group_class  -  limit the group class to the group file mask
  * @alloc:	acl and number of allocated entries
+ * @deny:	additional permissions to deny
  *
  * POSIX requires that after a chmod, the group class is granted no more
  * permissions than the group file permission bits.  For richacls, this
@@ -679,21 +687,20 @@ __richacl_isolate_who(struct richacl_alloc *alloc, struct richace *who,
  *    everyone@:rw::allow
  */
 static int
-richacl_isolate_group_class(struct richacl_alloc *alloc)
+richacl_isolate_group_class(struct richacl_alloc *alloc, unsigned int deny)
 {
 	struct richace who = {
 		.e_flags = RICHACE_SPECIAL_WHO,
 		.e_id.special = RICHACE_GROUP_SPECIAL_ID,
 	};
 	struct richace *ace;
-	unsigned int deny;
 
 	if (!alloc->acl->a_count)
 		return 0;
 	ace = alloc->acl->a_entries + alloc->acl->a_count - 1;
 	if (richace_is_inherit_only(ace) || !richace_is_everyone(ace))
 		return 0;
-	deny = ace->e_mask & ~alloc->acl->a_group_mask;
+	deny |= ace->e_mask & ~alloc->acl->a_group_mask;
 
 	if (deny) {
 		unsigned int n;
@@ -806,16 +813,17 @@ richacl_apply_masks(struct richacl **acl, kuid_t owner)
 			.acl = richacl_clone(*acl, GFP_KERNEL),
 			.count = (*acl)->a_count,
 		};
+		unsigned int added = 0;
+
 		if (!alloc.acl)
 			return -ENOMEM;
-
 		if (richacl_move_everyone_aces_down(&alloc) ||
 		    richacl_propagate_everyone(&alloc) ||
 		    __richacl_apply_masks(&alloc, owner) ||
+		    richacl_set_other_permissions(&alloc, &added) ||
+		    richacl_isolate_group_class(&alloc, added) ||
 		    richacl_set_owner_permissions(&alloc) ||
-		    richacl_set_other_permissions(&alloc) ||
-		    richacl_isolate_owner_class(&alloc) ||
-		    richacl_isolate_group_class(&alloc)) {
+		    richacl_isolate_owner_class(&alloc)) {
 			richacl_put(alloc.acl);
 			return -ENOMEM;
 		}
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1233008 — Re: [PATCH] richacl: Possible other write-through fix

From"J. Bruce Fields" <bfields@fieldses.org>
Date2015-09-25 20:40 +0200
SubjectRe: [PATCH] richacl: Possible other write-through fix
Message-ID<qcG4y-176-9@gated-at.bofh.it>
In reply to#1232935
On Fri, Sep 25, 2015 at 06:45:59PM +0200, Andreas Gruenbacher wrote:
> 2015-09-24 20:33 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> > On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
> >> +int
> >> +richacl_apply_masks(struct richacl **acl, kuid_t owner)
> >> +{
> >> +     if ((*acl)->a_flags & RICHACL_MASKED) {
> >> +             struct richacl_alloc alloc = {
> >> +                     .acl = richacl_clone(*acl, GFP_KERNEL),
> >> +                     .count = (*acl)->a_count,
> >> +             };
> >> +             if (!alloc.acl)
> >> +                     return -ENOMEM;
> >> +
> >> +             if (richacl_move_everyone_aces_down(&alloc) ||
> >> +                 richacl_propagate_everyone(&alloc) ||
> >> +                 __richacl_apply_masks(&alloc, owner) ||
> >> +                 richacl_set_owner_permissions(&alloc) ||
> >> +                 richacl_set_other_permissions(&alloc) ||
> >
> > Hm, I'm a little unsure about this step: it seems like the one step that
> > can actually change the permissions (making them more permissive, in
> > this case), whereas the others are neutral.
> >
> > The following two steps should fix that, but I'm not sure they do.
> >
> > E.g. if I have an ACL like
> >
> >         mask 0777, WRITE_THROUGH
> >         GROUP@:r--::allow
> >
> > I think it should result in
> >
> >         OWNER@:   rwx::allow
> >         GROUP@:   -wx::deny
> >         GROUP@:   r--::allow
> >         EVERYONE@:rwx::allow
> >
> > But does the GROUP@ deny actually get in there?  It looks to me like the
> > denies inserted by richacl_isolate_group_class only take into account
> > the group mask, not the actual permissions granted to any group class
> > user.
> >
> > I may be mising something.
> 
> Thanks for the test case and analysis. The group@ deny ace that should be
> inserted here actually doesn't get inserted. I'm attaching a fix.

This looks correct with the fix, thanks!

One nit:

>  		if (richacl_move_everyone_aces_down(&alloc) ||
>  		    richacl_propagate_everyone(&alloc) ||
>  		    __richacl_apply_masks(&alloc, owner) ||
> +		    richacl_set_other_permissions(&alloc, &added) ||

It's still the case that this step lacks the property shared by the
other step, that it leaves permissions unchanged.  Instead it increases
permissions, then the following step fixes the problem.  That
complicates review slightly.

But I'm not sure that matters much.

Reviewed-by: J. Bruce Fields <bfields@redhat.com>

--b.

> +		    richacl_isolate_group_class(&alloc, added) ||
>  		    richacl_set_owner_permissions(&alloc) ||
> -		    richacl_set_other_permissions(&alloc) ||
> -		    richacl_isolate_owner_class(&alloc) ||
> -		    richacl_isolate_group_class(&alloc)) {
> +		    richacl_isolate_owner_class(&alloc)) {
>  			richacl_put(alloc.acl);
>  			return -ENOMEM;
>  		}
> -- 
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web