Re: [PATCH 1/4] security: filesystem capabilities bugfix1

[David Howells <dhowells@redhat.com>]

David Howells <dhowells@redhat.com> wrote:

> > +kernel_cap_t cap_set_effective(const kernel_cap_t pE_new)
> 
> Hmmm... kernel_cap_t is a structure that might not fit into a single register.
> It occurs to me that you might be better off returing the old caps through a
> pointer argument.

Apply something like the attached, perhaps?  (Note the attached patch is
missing the change from linux/capability.h because my version is not
compatible with your submitted patch).

David

diff --git a/fs/open.c b/fs/open.c
index 3b53948..24b4a11 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -451,9 +451,9 @@ asmlinkage long sys_faccessat(int dfd, const char __user *filename, int mode)
 		 */
 #endif /* ndef CONFIG_SECURITY_FILE_CAPABILITIES */
 		if (current->uid)
-			old_cap = cap_set_effective(__cap_empty_set);
+			cap_set_effective(&__cap_empty_set, &old_cap);
 		else
-			old_cap = cap_set_effective(current->cap_permitted);
+			cap_set_effective(&current->cap_permitted, &old_cap);
 	}
 
 	res = __user_walk_fd(dfd, filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd);
@@ -484,9 +484,8 @@ out:
 	current->fsuid = old_fsuid;
 	current->fsgid = old_fsgid;
 
-	if (!issecure(SECURE_NO_SETUID_FIXUP)) {
-		(void) cap_set_effective(old_cap);
-	}
+	if (!issecure(SECURE_NO_SETUID_FIXUP))
+		cap_set_effective(&old_cap, NULL);
 
 	return res;
 }
diff --git a/kernel/capability.c b/kernel/capability.c
index c3bf957..13c496a 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -126,18 +126,16 @@ static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
  * value. No permission check is performed here - it is assumed that the
  * caller is permitted to set the desired effective capabilities.
  */
-kernel_cap_t cap_set_effective(const kernel_cap_t pE_new)
+void cap_set_effective(const kernel_cap_t *pE_new,
+		       kernel_cap_t *_pE_old)
 {
-	kernel_cap_t pE_old;
-
 	spin_lock(&task_capability_lock);
 
-	pE_old = current->cap_effective;
-	current->cap_effective = pE_new;
+	if (_pE_old)
+		*_pE_old = current->cap_effective;
+	current->cap_effective = *pE_new;
 
 	spin_unlock(&task_capability_lock);
-
-	return pE_old;
 }
 
 EXPORT_SYMBOL(cap_set_effective);