| Message ID | 1441448856-13478-23-git-send-email-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote: > The trailing everyone@ allow ace can grant permissions to all file > classes including the owner and group class. Before we can apply the > other mask to this entry to turn it into an "other class" entry, we need > to ensure that members of the owner or group class will not lose any > permissions from that ace. > > Conceptually, we do this by inserting additional <who>:<allow>::allow > entries before the trailing everyone@ allow ace with the same > permissions as the trailing everyone@ allow ace for owner@, group@, and > all explicitly mentioned users and groups. (In practice, we will rarely > need to insert any additional aces in this step.) > > Signed-off-by: Andreas Gruenbacher <agruen@kernel.org> > --- > fs/richacl_compat.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 195 insertions(+) > > diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c > index 4f0acf5..9b76fc0 100644 > --- a/fs/richacl_compat.c > +++ b/fs/richacl_compat.c > @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc *alloc) > } > return 0; > } > + > +/** > + * __richacl_propagate_everyone - propagate everyone@ permissions up for @who > + * @alloc: acl and number of allocated entries > + * @who: identifier to propagate permissions for > + * @allow: permissions to propagate up > + * > + * Propagate the permissions in @allow up from the end of the acl to the start > + * for the specified principal @who. > + * > + * The simplest possible approach to achieve this would be to insert a > + * "<who>:<allow>::allow" ace before the final everyone@ allow ace. Since this > + * would often result in aces which are not needed or which could be merged > + * with an existing ace, we make the following optimizations: > + * > + * - We go through the acl and determine which permissions are already > + * allowed or denied to @who, and we remove those permissions from > + * @allow. > + * > + * - If the acl contains an allow ace for @who and no aces after this entry > + * deny permissions in @allow, we add the permissions in @allow to this > + * ace. (Propagating permissions across a deny ace which can match the > + * process can elevate permissions.) > + * > + * This transformation does not alter the permissions that the acl grants. > + */ > +static int > +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace *who, > + unsigned int allow) > +{ > + struct richace *allow_last = NULL, *ace; > + struct richacl *acl = alloc->acl; > + > + /* > + * Remove the permissions from allow that are already determined for > + * this who value, and figure out if there is an allow entry for > + * this who value that is "reachable" from the trailing everyone@ > + * allow ace. > + */ > + richacl_for_each_entry(ace, acl) { > + if (richace_is_inherit_only(ace)) > + continue; > + if (richace_is_allow(ace)) { > + if (richace_is_same_identifier(ace, who)) { > + allow &= ~ace->e_mask; > + allow_last = ace; > + } > + } else if (richace_is_deny(ace)) { > + if (richace_is_same_identifier(ace, who)) > + allow &= ~ace->e_mask; > + else if (allow & ace->e_mask) > + allow_last = NULL; > + } > + } > + ace--; > + > + /* > + * If for group class entries, all the remaining permissions will > + * remain granted by the trailing everyone@ ace, no additional entry is > + * needed. > + */ > + if (!richace_is_owner(who) && > + richace_is_everyone(ace) && richace_is_allow(ace) && That richace_is_allow(ace) check is redundant at this point, isn't it? > + !(allow & ~(ace->e_mask & acl->a_other_mask))) Uh, I wish C had a subset-of operator, that construct took me longer to work out than I should admit. > + allow = 0; > + > + if (allow) { > + if (allow_last) > + return richace_change_mask(alloc, &allow_last, > + allow_last->e_mask | allow); > + else { > + struct richace who_copy; > + > + richace_copy(&who_copy, who); > + ace = acl->a_entries + acl->a_count - 1; Isn't ace already set to the last ace? --b. > + if (richacl_insert_entry(alloc, &ace)) > + return -1; > + richace_copy(ace, &who_copy); > + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE; > + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS; > + ace->e_mask = allow; > + } > + } > + return 0; > +} > + > +/** > + * richacl_propagate_everyone - propagate everyone@ permissions up the acl > + * @alloc: acl and number of allocated entries > + * > + * Make sure that group@ and all other users and groups mentioned in the acl > + * will not lose any permissions when finally applying the other mask to the > + * everyone@ allow ace at the end of the acl. We modify the permissions of > + * existing entries or add new entries before the final everyone@ allow ace to > + * achieve that. > + * > + * For example, the following acl implicitly grants everyone rwpx access: > + * > + * joe:r::allow > + * everyone@:rwpx::allow > + * > + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe > + * would lose wp access even though the mode does not exclude those > + * permissions. After propagating the everyone@ permissions, the result for > + * applying mode 0660 becomes: > + * > + * owner@:rwp::allow > + * joe:rwp::allow > + * group@:rwp::allow > + * > + * Deny aces complicate the matter. For example, the following acl grants > + * everyone but joe write access: > + * > + * joe:wp::deny > + * everyone@:rwpx::allow > + * > + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe > + * would lose r access. After propagating the everyone@ permissions, the > + * result for applying mode 0660 becomes: > + * > + * owner@:rwp::allow > + * joe:w::deny > + * group@:rwp::allow > + * joe:r::allow > + */ > +static int > +richacl_propagate_everyone(struct richacl_alloc *alloc) > +{ > + struct richace who = { .e_flags = RICHACE_SPECIAL_WHO }; > + struct richacl *acl = alloc->acl; > + struct richace *ace; > + unsigned int owner_allow, group_allow; > + > + /* > + * If the owner mask contains permissions which are not in the group > + * mask, the group mask contains permissions which are not in the other > + * mask, or the owner class contains permissions which are not in the > + * other mask, we may need to propagate permissions up from the > + * everyone@ allow ace. The third condition is implied by the first > + * two. > + */ > + if (!((acl->a_owner_mask & ~acl->a_group_mask) || > + (acl->a_group_mask & ~acl->a_other_mask))) > + return 0; > + if (!acl->a_count) > + return 0; > + ace = acl->a_entries + acl->a_count - 1; > + if (richace_is_inherit_only(ace) || !richace_is_everyone(ace)) > + return 0; > + > + owner_allow = ace->e_mask & acl->a_owner_mask; > + group_allow = ace->e_mask & acl->a_group_mask; > + > + if (owner_allow & ~(acl->a_group_mask & acl->a_other_mask)) { > + /* Propagate everyone@ permissions through to owner@. */ > + who.e_id.special = RICHACE_OWNER_SPECIAL_ID; > + if (__richacl_propagate_everyone(alloc, &who, owner_allow)) > + return -1; > + acl = alloc->acl; > + } > + > + if (group_allow & ~acl->a_other_mask) { > + int n; > + > + /* Propagate everyone@ permissions through to group@. */ > + who.e_id.special = RICHACE_GROUP_SPECIAL_ID; > + if (__richacl_propagate_everyone(alloc, &who, group_allow)) > + return -1; > + acl = alloc->acl; > + > + /* > + * Start from the entry before the trailing everyone@ allow > + * entry. We will not hit everyone@ entries in the loop. > + */ > + for (n = acl->a_count - 2; n != -1; n--) { > + ace = acl->a_entries + n; > + > + if (richace_is_inherit_only(ace) || > + richace_is_owner(ace) || > + richace_is_group(ace)) > + continue; > + if (richace_is_allow(ace) || richace_is_deny(ace)) { > + /* > + * Any inserted entry will end up below the > + * current entry > + */ > + if (__richacl_propagate_everyone(alloc, ace, > + group_allow)) > + return -1; > + acl = alloc->acl; > + } > + } > + } > + return 0; > +} > -- > 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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote: > The trailing everyone@ allow ace can grant permissions to all file > classes including the owner and group class. Before we can apply the > other mask to this entry to turn it into an "other class" entry, we need > to ensure that members of the owner or group class will not lose any > permissions from that ace. > > Conceptually, we do this by inserting additional <who>:<allow>::allow > entries before the trailing everyone@ allow ace with the same > permissions as the trailing everyone@ allow ace for owner@, group@, and > all explicitly mentioned users and groups. (In practice, we will rarely > need to insert any additional aces in this step.) > > Signed-off-by: Andreas Gruenbacher <agruen@kernel.org> > --- > fs/richacl_compat.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 195 insertions(+) > > diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c > index 4f0acf5..9b76fc0 100644 > --- a/fs/richacl_compat.c > +++ b/fs/richacl_compat.c > @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc *alloc) > } > return 0; > } > + > +/** > + * __richacl_propagate_everyone - propagate everyone@ permissions up for @who > + * @alloc: acl and number of allocated entries > + * @who: identifier to propagate permissions for > + * @allow: permissions to propagate up > + * > + * Propagate the permissions in @allow up from the end of the acl to the start > + * for the specified principal @who. > + * > + * The simplest possible approach to achieve this would be to insert a > + * "<who>:<allow>::allow" ace before the final everyone@ allow ace. Since this > + * would often result in aces which are not needed or which could be merged > + * with an existing ace, we make the following optimizations: > + * > + * - We go through the acl and determine which permissions are already > + * allowed or denied to @who, and we remove those permissions from > + * @allow. > + * > + * - If the acl contains an allow ace for @who and no aces after this entry > + * deny permissions in @allow, we add the permissions in @allow to this > + * ace. (Propagating permissions across a deny ace which can match the > + * process can elevate permissions.) > + * > + * This transformation does not alter the permissions that the acl grants. > + */ > +static int > +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace *who, > + unsigned int allow) > +{ > + struct richace *allow_last = NULL, *ace; > + struct richacl *acl = alloc->acl; > + > + /* > + * Remove the permissions from allow that are already determined for > + * this who value, and figure out if there is an allow entry for > + * this who value that is "reachable" from the trailing everyone@ > + * allow ace. > + */ > + richacl_for_each_entry(ace, acl) { > + if (richace_is_inherit_only(ace)) > + continue; > + if (richace_is_allow(ace)) { > + if (richace_is_same_identifier(ace, who)) { > + allow &= ~ace->e_mask; > + allow_last = ace; > + } > + } else if (richace_is_deny(ace)) { > + if (richace_is_same_identifier(ace, who)) > + allow &= ~ace->e_mask; > + else if (allow & ace->e_mask) > + allow_last = NULL; > + } > + } > + ace--; > + > + /* > + * If for group class entries, all the remaining permissions will > + * remain granted by the trailing everyone@ ace, no additional entry is > + * needed. > + */ > + if (!richace_is_owner(who) && > + richace_is_everyone(ace) && richace_is_allow(ace) && > + !(allow & ~(ace->e_mask & acl->a_other_mask))) > + allow = 0; > + > + if (allow) { > + if (allow_last) > + return richace_change_mask(alloc, &allow_last, > + allow_last->e_mask | allow); > + else { > + struct richace who_copy; > + > + richace_copy(&who_copy, who); > + ace = acl->a_entries + acl->a_count - 1; > + if (richacl_insert_entry(alloc, &ace)) > + return -1; > + richace_copy(ace, &who_copy); > + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE; > + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS; > + ace->e_mask = allow; > + } > + } > + return 0; > +} > + > +/** > + * richacl_propagate_everyone - propagate everyone@ permissions up the acl > + * @alloc: acl and number of allocated entries > + * > + * Make sure that group@ and all other users and groups mentioned in the acl > + * will not lose any permissions when finally applying the other mask to the > + * everyone@ allow ace at the end of the acl. We modify the permissions of > + * existing entries or add new entries before the final everyone@ allow ace to > + * achieve that. > + * > + * For example, the following acl implicitly grants everyone rwpx access: > + * > + * joe:r::allow > + * everyone@:rwpx::allow > + * > + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe > + * would lose wp access even though the mode does not exclude those > + * permissions. After propagating the everyone@ permissions, the result for > + * applying mode 0660 becomes: > + * > + * owner@:rwp::allow > + * joe:rwp::allow > + * group@:rwp::allow > + * > + * Deny aces complicate the matter. For example, the following acl grants > + * everyone but joe write access: > + * > + * joe:wp::deny > + * everyone@:rwpx::allow > + * > + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe > + * would lose r access. After propagating the everyone@ permissions, the > + * result for applying mode 0660 becomes: > + * > + * owner@:rwp::allow > + * joe:w::deny > + * group@:rwp::allow > + * joe:r::allow > + */ > +static int > +richacl_propagate_everyone(struct richacl_alloc *alloc) > +{ > + struct richace who = { .e_flags = RICHACE_SPECIAL_WHO }; > + struct richacl *acl = alloc->acl; > + struct richace *ace; > + unsigned int owner_allow, group_allow; > + > + /* > + * If the owner mask contains permissions which are not in the group > + * mask, the group mask contains permissions which are not in the other > + * mask, or the owner class contains permissions which are not in the s/owner class/owner mask? > + * other mask, we may need to propagate permissions up from the > + * everyone@ allow ace. The third condition is implied by the first > + * two. > + */ > + if (!((acl->a_owner_mask & ~acl->a_group_mask) || > + (acl->a_group_mask & ~acl->a_other_mask))) > + return 0; The code looks right, but I don't understand the preceding comment. For example, owner mask: rw group mask: wx other mask: rw satisfies the first two conditions, but not the third. Also, I don't understand why the first condition would imply that we might need to propagate permissions. --b. > + if (!acl->a_count) > + return 0; > + ace = acl->a_entries + acl->a_count - 1; > + if (richace_is_inherit_only(ace) || !richace_is_everyone(ace)) > + return 0; > + > + owner_allow = ace->e_mask & acl->a_owner_mask; > + group_allow = ace->e_mask & acl->a_group_mask; > + > + if (owner_allow & ~(acl->a_group_mask & acl->a_other_mask)) { > + /* Propagate everyone@ permissions through to owner@. */ > + who.e_id.special = RICHACE_OWNER_SPECIAL_ID; > + if (__richacl_propagate_everyone(alloc, &who, owner_allow)) > + return -1; > + acl = alloc->acl; > + } > + > + if (group_allow & ~acl->a_other_mask) { > + int n; > + > + /* Propagate everyone@ permissions through to group@. */ > + who.e_id.special = RICHACE_GROUP_SPECIAL_ID; > + if (__richacl_propagate_everyone(alloc, &who, group_allow)) > + return -1; > + acl = alloc->acl; > + > + /* > + * Start from the entry before the trailing everyone@ allow > + * entry. We will not hit everyone@ entries in the loop. > + */ > + for (n = acl->a_count - 2; n != -1; n--) { > + ace = acl->a_entries + n; > + > + if (richace_is_inherit_only(ace) || > + richace_is_owner(ace) || > + richace_is_group(ace)) > + continue; > + if (richace_is_allow(ace) || richace_is_deny(ace)) { > + /* > + * Any inserted entry will end up below the > + * current entry > + */ > + if (__richacl_propagate_everyone(alloc, ace, > + group_allow)) > + return -1; > + acl = alloc->acl; > + } > + } > + } > + return 0; > +} > -- > 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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 18, 2015 at 05:56:11PM -0400, bfields wrote: > On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote: > > The trailing everyone@ allow ace can grant permissions to all file > > classes including the owner and group class. Before we can apply the > > other mask to this entry to turn it into an "other class" entry, we need > > to ensure that members of the owner or group class will not lose any > > permissions from that ace. > > > > Conceptually, we do this by inserting additional <who>:<allow>::allow > > entries before the trailing everyone@ allow ace with the same > > permissions as the trailing everyone@ allow ace for owner@, group@, and > > all explicitly mentioned users and groups. (In practice, we will rarely > > need to insert any additional aces in this step.) > > > > Signed-off-by: Andreas Gruenbacher <agruen@kernel.org> > > --- > > fs/richacl_compat.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 195 insertions(+) > > > > diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c > > index 4f0acf5..9b76fc0 100644 > > --- a/fs/richacl_compat.c > > +++ b/fs/richacl_compat.c > > @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc *alloc) > > } > > return 0; > > } > > + > > +/** > > + * __richacl_propagate_everyone - propagate everyone@ permissions up for @who > > + * @alloc: acl and number of allocated entries > > + * @who: identifier to propagate permissions for > > + * @allow: permissions to propagate up > > + * > > + * Propagate the permissions in @allow up from the end of the acl to the start > > + * for the specified principal @who. > > + * > > + * The simplest possible approach to achieve this would be to insert a > > + * "<who>:<allow>::allow" ace before the final everyone@ allow ace. Since this > > + * would often result in aces which are not needed or which could be merged > > + * with an existing ace, we make the following optimizations: > > + * > > + * - We go through the acl and determine which permissions are already > > + * allowed or denied to @who, and we remove those permissions from > > + * @allow. > > + * > > + * - If the acl contains an allow ace for @who and no aces after this entry > > + * deny permissions in @allow, we add the permissions in @allow to this > > + * ace. (Propagating permissions across a deny ace which can match the > > + * process can elevate permissions.) > > + * > > + * This transformation does not alter the permissions that the acl grants. > > + */ > > +static int > > +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace *who, > > + unsigned int allow) > > +{ > > + struct richace *allow_last = NULL, *ace; > > + struct richacl *acl = alloc->acl; > > + > > + /* > > + * Remove the permissions from allow that are already determined for > > + * this who value, and figure out if there is an allow entry for > > + * this who value that is "reachable" from the trailing everyone@ > > + * allow ace. > > + */ > > + richacl_for_each_entry(ace, acl) { > > + if (richace_is_inherit_only(ace)) > > + continue; > > + if (richace_is_allow(ace)) { > > + if (richace_is_same_identifier(ace, who)) { > > + allow &= ~ace->e_mask; > > + allow_last = ace; > > + } > > + } else if (richace_is_deny(ace)) { > > + if (richace_is_same_identifier(ace, who)) > > + allow &= ~ace->e_mask; > > + else if (allow & ace->e_mask) > > + allow_last = NULL; > > + } > > + } > > + ace--; > > + > > + /* > > + * If for group class entries, all the remaining permissions will > > + * remain granted by the trailing everyone@ ace, no additional entry is > > + * needed. > > + */ > > + if (!richace_is_owner(who) && > > + richace_is_everyone(ace) && richace_is_allow(ace) && > > + !(allow & ~(ace->e_mask & acl->a_other_mask))) > > + allow = 0; > > + > > + if (allow) { > > + if (allow_last) > > + return richace_change_mask(alloc, &allow_last, > > + allow_last->e_mask | allow); > > + else { > > + struct richace who_copy; > > + > > + richace_copy(&who_copy, who); > > + ace = acl->a_entries + acl->a_count - 1; > > + if (richacl_insert_entry(alloc, &ace)) > > + return -1; > > + richace_copy(ace, &who_copy); > > + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE; > > + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS; > > + ace->e_mask = allow; > > + } > > + } > > + return 0; > > +} > > + > > +/** > > + * richacl_propagate_everyone - propagate everyone@ permissions up the acl > > + * @alloc: acl and number of allocated entries > > + * > > + * Make sure that group@ and all other users and groups mentioned in the acl > > + * will not lose any permissions when finally applying the other mask to the > > + * everyone@ allow ace at the end of the acl. We modify the permissions of > > + * existing entries or add new entries before the final everyone@ allow ace to > > + * achieve that. > > + * > > + * For example, the following acl implicitly grants everyone rwpx access: > > + * > > + * joe:r::allow > > + * everyone@:rwpx::allow > > + * > > + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe > > + * would lose wp access even though the mode does not exclude those > > + * permissions. After propagating the everyone@ permissions, the result for > > + * applying mode 0660 becomes: > > + * > > + * owner@:rwp::allow > > + * joe:rwp::allow > > + * group@:rwp::allow > > + * > > + * Deny aces complicate the matter. For example, the following acl grants > > + * everyone but joe write access: > > + * > > + * joe:wp::deny > > + * everyone@:rwpx::allow > > + * > > + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe > > + * would lose r access. After propagating the everyone@ permissions, the > > + * result for applying mode 0660 becomes: > > + * > > + * owner@:rwp::allow > > + * joe:w::deny > > + * group@:rwp::allow > > + * joe:r::allow > > + */ > > +static int > > +richacl_propagate_everyone(struct richacl_alloc *alloc) > > +{ > > + struct richace who = { .e_flags = RICHACE_SPECIAL_WHO }; > > + struct richacl *acl = alloc->acl; > > + struct richace *ace; > > + unsigned int owner_allow, group_allow; > > + > > + /* > > + * If the owner mask contains permissions which are not in the group > > + * mask, the group mask contains permissions which are not in the other > > + * mask, or the owner class contains permissions which are not in the > > s/owner class/owner mask? > > > + * other mask, we may need to propagate permissions up from the > > + * everyone@ allow ace. The third condition is implied by the first > > + * two. > > + */ > > + if (!((acl->a_owner_mask & ~acl->a_group_mask) || > > + (acl->a_group_mask & ~acl->a_other_mask))) > > + return 0; > > The code looks right, but I don't understand the preceding comment. > > For example, > > owner mask: rw > group mask: wx > other mask: rw > > satisfies the first two conditions, but not the third. > > Also, I don't understand why the first condition would imply that we > might need to propagate permissions. OK, maybe I get the part about the owner mask containing permissions not in the group mask: we'll need to insert a deny ace for the bits in the other mask but not in the group mask, and then we'll need an allow ace for the owner to get those bits back. I think? > > + if (richace_is_allow(ace) || richace_is_deny(ace)) { The v4 spec allows aces other than allow and deny aces (audit and alarm), but I didn't think you were implementing those. --b. -- 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
2015-09-18 23:36 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>: > On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote: >> + if (!richace_is_owner(who) && >> + richace_is_everyone(ace) && richace_is_allow(ace) && > > That richace_is_allow(ace) check is redundant at this point, isn't it? Yes, I'll change that. >> + !(allow & ~(ace->e_mask & acl->a_other_mask))) > > Uh, I wish C had a subset-of operator, that construct took me longer to > work out than I should admit. > >> + allow = 0; >> + >> + if (allow) { >> + if (allow_last) >> + return richace_change_mask(alloc, &allow_last, >> + allow_last->e_mask | allow); >> + else { >> + struct richace who_copy; >> + >> + richace_copy(&who_copy, who); >> + ace = acl->a_entries + acl->a_count - 1; > > Isn't ace already set to the last ace? Yes indeed, that line can also go. Thanks, Andreas -- 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
2015-09-21 21:24 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>: > On Fri, Sep 18, 2015 at 05:56:11PM -0400, bfields wrote: >> On Sat, Sep 05, 2015 at 12:27:17PM +0200, Andreas Gruenbacher wrote: >> > + /* >> > + * If the owner mask contains permissions which are not in the group >> > + * mask, the group mask contains permissions which are not in the other >> > + * mask, or the owner class contains permissions which are not in the >> >> s/owner class/owner mask? >> >> > + * other mask, we may need to propagate permissions up from the >> > + * everyone@ allow ace. The third condition is implied by the first >> > + * two. >> > + */ >> > + if (!((acl->a_owner_mask & ~acl->a_group_mask) || >> > + (acl->a_group_mask & ~acl->a_other_mask))) >> > + return 0; >> >> The code looks right, but I don't understand the preceding comment. >> >> For example, >> >> owner mask: rw >> group mask: wx >> other mask: rw >> >> satisfies the first two conditions, but not the third. >> >> Also, I don't understand why the first condition would imply that we >> might need to propagate permissions. > > OK, maybe I get the part about the owner mask containing permissions > not in the group mask: we'll need to insert a deny ace for the bits in > the other mask but not in the group mask, and then we'll need an allow > ace for the owner to get those bits back. I think? That is indeed the reason, and it also seems clear that this wasn't documented well enough. Let me remove the offending comment and tiny optimization, and add better comments instead. >> > + if (richace_is_allow(ace) || richace_is_deny(ace)) { > > The v4 spec allows aces other than allow and deny aces (audit and > alarm), but I didn't think you were implementing those. Right, I don't see that happening. I'll remove that as well. Thanks, Andreas -- 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
diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c index 4f0acf5..9b76fc0 100644 --- a/fs/richacl_compat.c +++ b/fs/richacl_compat.c @@ -218,3 +218,198 @@ richacl_move_everyone_aces_down(struct richacl_alloc *alloc) } return 0; } + +/** + * __richacl_propagate_everyone - propagate everyone@ permissions up for @who + * @alloc: acl and number of allocated entries + * @who: identifier to propagate permissions for + * @allow: permissions to propagate up + * + * Propagate the permissions in @allow up from the end of the acl to the start + * for the specified principal @who. + * + * The simplest possible approach to achieve this would be to insert a + * "<who>:<allow>::allow" ace before the final everyone@ allow ace. Since this + * would often result in aces which are not needed or which could be merged + * with an existing ace, we make the following optimizations: + * + * - We go through the acl and determine which permissions are already + * allowed or denied to @who, and we remove those permissions from + * @allow. + * + * - If the acl contains an allow ace for @who and no aces after this entry + * deny permissions in @allow, we add the permissions in @allow to this + * ace. (Propagating permissions across a deny ace which can match the + * process can elevate permissions.) + * + * This transformation does not alter the permissions that the acl grants. + */ +static int +__richacl_propagate_everyone(struct richacl_alloc *alloc, struct richace *who, + unsigned int allow) +{ + struct richace *allow_last = NULL, *ace; + struct richacl *acl = alloc->acl; + + /* + * Remove the permissions from allow that are already determined for + * this who value, and figure out if there is an allow entry for + * this who value that is "reachable" from the trailing everyone@ + * allow ace. + */ + richacl_for_each_entry(ace, acl) { + if (richace_is_inherit_only(ace)) + continue; + if (richace_is_allow(ace)) { + if (richace_is_same_identifier(ace, who)) { + allow &= ~ace->e_mask; + allow_last = ace; + } + } else if (richace_is_deny(ace)) { + if (richace_is_same_identifier(ace, who)) + allow &= ~ace->e_mask; + else if (allow & ace->e_mask) + allow_last = NULL; + } + } + ace--; + + /* + * If for group class entries, all the remaining permissions will + * remain granted by the trailing everyone@ ace, no additional entry is + * needed. + */ + if (!richace_is_owner(who) && + richace_is_everyone(ace) && richace_is_allow(ace) && + !(allow & ~(ace->e_mask & acl->a_other_mask))) + allow = 0; + + if (allow) { + if (allow_last) + return richace_change_mask(alloc, &allow_last, + allow_last->e_mask | allow); + else { + struct richace who_copy; + + richace_copy(&who_copy, who); + ace = acl->a_entries + acl->a_count - 1; + if (richacl_insert_entry(alloc, &ace)) + return -1; + richace_copy(ace, &who_copy); + ace->e_type = RICHACE_ACCESS_ALLOWED_ACE_TYPE; + ace->e_flags &= ~RICHACE_INHERITANCE_FLAGS; + ace->e_mask = allow; + } + } + return 0; +} + +/** + * richacl_propagate_everyone - propagate everyone@ permissions up the acl + * @alloc: acl and number of allocated entries + * + * Make sure that group@ and all other users and groups mentioned in the acl + * will not lose any permissions when finally applying the other mask to the + * everyone@ allow ace at the end of the acl. We modify the permissions of + * existing entries or add new entries before the final everyone@ allow ace to + * achieve that. + * + * For example, the following acl implicitly grants everyone rwpx access: + * + * joe:r::allow + * everyone@:rwpx::allow + * + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe + * would lose wp access even though the mode does not exclude those + * permissions. After propagating the everyone@ permissions, the result for + * applying mode 0660 becomes: + * + * owner@:rwp::allow + * joe:rwp::allow + * group@:rwp::allow + * + * Deny aces complicate the matter. For example, the following acl grants + * everyone but joe write access: + * + * joe:wp::deny + * everyone@:rwpx::allow + * + * When applying mode 0660 to this acl, group@ would lose rwp access, and joe + * would lose r access. After propagating the everyone@ permissions, the + * result for applying mode 0660 becomes: + * + * owner@:rwp::allow + * joe:w::deny + * group@:rwp::allow + * joe:r::allow + */ +static int +richacl_propagate_everyone(struct richacl_alloc *alloc) +{ + struct richace who = { .e_flags = RICHACE_SPECIAL_WHO }; + struct richacl *acl = alloc->acl; + struct richace *ace; + unsigned int owner_allow, group_allow; + + /* + * If the owner mask contains permissions which are not in the group + * mask, the group mask contains permissions which are not in the other + * mask, or the owner class contains permissions which are not in the + * other mask, we may need to propagate permissions up from the + * everyone@ allow ace. The third condition is implied by the first + * two. + */ + if (!((acl->a_owner_mask & ~acl->a_group_mask) || + (acl->a_group_mask & ~acl->a_other_mask))) + return 0; + if (!acl->a_count) + return 0; + ace = acl->a_entries + acl->a_count - 1; + if (richace_is_inherit_only(ace) || !richace_is_everyone(ace)) + return 0; + + owner_allow = ace->e_mask & acl->a_owner_mask; + group_allow = ace->e_mask & acl->a_group_mask; + + if (owner_allow & ~(acl->a_group_mask & acl->a_other_mask)) { + /* Propagate everyone@ permissions through to owner@. */ + who.e_id.special = RICHACE_OWNER_SPECIAL_ID; + if (__richacl_propagate_everyone(alloc, &who, owner_allow)) + return -1; + acl = alloc->acl; + } + + if (group_allow & ~acl->a_other_mask) { + int n; + + /* Propagate everyone@ permissions through to group@. */ + who.e_id.special = RICHACE_GROUP_SPECIAL_ID; + if (__richacl_propagate_everyone(alloc, &who, group_allow)) + return -1; + acl = alloc->acl; + + /* + * Start from the entry before the trailing everyone@ allow + * entry. We will not hit everyone@ entries in the loop. + */ + for (n = acl->a_count - 2; n != -1; n--) { + ace = acl->a_entries + n; + + if (richace_is_inherit_only(ace) || + richace_is_owner(ace) || + richace_is_group(ace)) + continue; + if (richace_is_allow(ace) || richace_is_deny(ace)) { + /* + * Any inserted entry will end up below the + * current entry + */ + if (__richacl_propagate_everyone(alloc, ace, + group_allow)) + return -1; + acl = alloc->acl; + } + } + } + return 0; +}
The trailing everyone@ allow ace can grant permissions to all file classes including the owner and group class. Before we can apply the other mask to this entry to turn it into an "other class" entry, we need to ensure that members of the owner or group class will not lose any permissions from that ace. Conceptually, we do this by inserting additional <who>:<allow>::allow entries before the trailing everyone@ allow ace with the same permissions as the trailing everyone@ allow ace for owner@, group@, and all explicitly mentioned users and groups. (In practice, we will rarely need to insert any additional aces in this step.) Signed-off-by: Andreas Gruenbacher <agruen@kernel.org> --- fs/richacl_compat.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+)