On Fri, Sep 25, 2015 at 06:45:59PM +0200, Andreas Gruenbacher wrote:

2015-09-24 20:33 GMT+02:00 J. Bruce Fields [off-list ref]:

quoted

On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:

quoted

+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 <redacted>

--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