Skip to contentSkip to conversation

2.5.41 capget fix

PATCH In review Chris Wright 3 participants 7 messages Oct 10, 2002 – Nov 20, 2002

Subsystems (2)
Security (LSM) Capabilities
lore
Share & tools
lore

Chris Wright

author
#0 lore raw

PATCH 1/1 2.5.41 capget fix

Linus,

Daniel Jacobowitz noticed that sys_capget is not behaving properly when called with pid of 0. It is supposed to return current capabilities, not those of swapper. Also cleaned up some duplicate code from a merge error. Patch is tested, please apply.

thanks,
-chris

kernel/capability.c +9 -10
@@ -33,7 +33,7 @@
      int ret = 0;
      pid_t pid;
      __u32 version;
-     task_t *target;
+     task_t *target = current;
      struct __user_cap_data_struct data;
 
      if (get_user(version, &header->version))

@@ -52,21 +52,20 @@
              return -EINVAL;
 
      spin_lock(&task_capability_lock);
-     read_lock(&tasklist_lock); 
 
-     target = find_task_by_pid(pid);
-     if (!target) {
-          ret = -ESRCH;
-          goto out;
+     if (pid && pid != current->pid) {
+	     read_lock(&tasklist_lock); 
+	     target = find_task_by_pid(pid);
+	     read_unlock(&tasklist_lock); 
+	     if (!target) {
+	          ret = -ESRCH;
+	          goto out;
+	     }
      }
 
-     data.permitted = cap_t(target->cap_permitted);
-     data.inheritable = cap_t(target->cap_inheritable); 
-     data.effective = cap_t(target->cap_effective);
      ret = security_ops->capget(target, &data.effective, &data.inheritable, &data.permitted);
 
 out:
-     read_unlock(&tasklist_lock); 
      spin_unlock(&task_capability_lock);
 
      if (!ret && copy_to_user(dataptr, &data, sizeof data))
Signature · 1 line
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net
#1 lore raw
Quoted 5 lines · Chris Wright

Daniel Jacobowitz noticed that sys_capget is not behaving properly when called with pid of 0. It is supposed to return current capabilities, not those of swapper. Also cleaned up some duplicate code from a merge error. Patch is tested, please apply.

This is not correct. You drop the tasklist_lock before you actually set read the capabilities, which means that by the time you read them, the task you looked up may not be there any more.

Linus

author
#2 lore raw
Quoted 10 lines · * Linus Torvalds ([email protected]) wrote:
Quoted 5 lines · Chris Wright

Daniel Jacobowitz noticed that sys_capget is not behaving properly when called with pid of 0. It is supposed to return current capabilities, not those of swapper. Also cleaned up some duplicate code from a merge error. Patch is tested, please apply.

This is not correct. You drop the tasklist_lock before you actually set read the capabilities, which means that by the time you read them, the task you looked up may not be there any more.

Doh...feeling dumb...

The patch below fixes my oversight. The locking is left the way it was, and just the pid 0 part is fixed as well as the duplicate code removed. This still applies against bk-curr and I tested it on 2.5.41.

thanks,
-chris

kernel/capability.c +7 -8
@@ -33,7 +33,7 @@
      int ret = 0;
      pid_t pid;
      __u32 version;
-     task_t *target;
+     task_t *target = current;
      struct __user_cap_data_struct data;
 
      if (get_user(version, &header->version))

@@ -54,15 +54,14 @@
      spin_lock(&task_capability_lock);
      read_lock(&tasklist_lock); 
 
-     target = find_task_by_pid(pid);
-     if (!target) {
-          ret = -ESRCH;
-          goto out;
+     if (pid && pid != current->pid) {
+	     target = find_task_by_pid(pid);
+	     if (!target) {
+	          ret = -ESRCH;
+	          goto out;
+	     }
      }
 
-     data.permitted = cap_t(target->cap_permitted);
-     data.inheritable = cap_t(target->cap_inheritable); 
-     data.effective = cap_t(target->cap_effective);
      ret = security_ops->capget(target, &data.effective, &data.inheritable, &data.permitted);
 
 out:
#3 lore raw
Quoted 3 lines · Chris Wright

The patch below fixes my oversight. The locking is left the way it was, and just the pid 0 part is fixed as well as the duplicate code removed.

All right, call me stupid, but twhere is the "duplication" in the code you removed?

I see the "security/capability.c" thing, yes, but I also look at "security/dummy.c", and it appears that at least for that case nobody would ever initialize the capabilities that we return to user space at all.

So there's a bug somewhere there, and removing the duplication makes things worse (admittedly for a case which isn't enabled in the regular kernel, but still..)

So I'd ask you to have patience with me, and send a third patch that gets this thing right too..

Linus

#4 lore raw

PATCH 1/1 sys_capget should use current if the pid argument is 0

Quoted 14 lines · Linus Torvalds
Quoted 3 lines · Chris Wright

The patch below fixes my oversight. The locking is left the way it was, and just the pid 0 part is fixed as well as the duplicate code removed.

All right, call me stupid, but twhere is the "duplication" in the code you removed?

I see the "security/capability.c" thing, yes, but I also look at "security/dummy.c", and it appears that at least for that case nobody would ever initialize the capabilities that we return to user space at all.

Quoted 6 lines

So there's a bug somewhere there, and removing the duplication makes things worse (admittedly for a case which isn't enabled in the regular kernel, but still..)

So I'd ask you to have patience with me, and send a third patch that gets this thing right too..

Since this is still broken a month later... I don't know what to do about the "duplication" question, but I'll leave that to Chris. This is the uncontroversial portion of Chris's patch; its affect is to change my zsh shell prompt back to a '%' as I'd expect.

Linus, please apply.

[PATCH] sys_capget should use current if the pid argument is 0

===== kernel/capability.c 1.6 vs edited =====

kernel/capability.c +7 -5
@@ -33,7 +33,7 @@
      int ret = 0;
      pid_t pid;
      __u32 version;
-     task_t *target;
+     task_t *target = current;
      struct __user_cap_data_struct data;
 
      if (get_user(version, &header->version))

@@ -54,10 +54,12 @@
      spin_lock(&task_capability_lock);
      read_lock(&tasklist_lock); 
 
-     target = find_task_by_pid(pid);
-     if (!target) {
-          ret = -ESRCH;
-          goto out;
+     if (pid && pid != current->pid) {
+             target = find_task_by_pid(pid);
+             if (!target) {
+                  ret = -ESRCH;
+                  goto out;
+            }
      }
 
      data.permitted = cap_t(target->cap_permitted);
Signature · 2 lines
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer
author
#5 lore raw

Re: [PATCH] sys_capget should use current if the pid argument is 0

Quoted 19 lines · * Daniel Jacobowitz ([email protected]) wrote:
Quoted 18 lines · Linus Torvalds
Quoted 3 lines · Chris Wright

The patch below fixes my oversight. The locking is left the way it was, and just the pid 0 part is fixed as well as the duplicate code removed.

All right, call me stupid, but twhere is the "duplication" in the code you removed?

I see the "security/capability.c" thing, yes, but I also look at "security/dummy.c", and it appears that at least for that case nobody would ever initialize the capabilities that we return to user space at all.

So there's a bug somewhere there, and removing the duplication makes things worse (admittedly for a case which isn't enabled in the regular kernel, but still..)

Quoted 7 lines
Quoted 2 lines

So I'd ask you to have patience with me, and send a third patch that gets this thing right too..

Since this is still broken a month later... I don't know what to do about the "duplication" question, but I'll leave that to Chris. This is the uncontroversial portion of Chris's patch; its affect is to change my zsh shell prompt back to a '%' as I'd expect.

Thanks Daniel. I've been using the patch below, which differs only slightly from the one you posted (code style matches the same lookup in sys_capset). I'll follow up with the patch that removes the duplicate code.

[PATCH] sys_capget should use current if the pid argument is 0

===== kernel/capability.c 1.5 vs edited =====

kernel/capability.c +8 -5
@@ -54,11 +54,14 @@
      spin_lock(&task_capability_lock);
      read_lock(&tasklist_lock); 
 
-     target = find_task_by_pid(pid);
-     if (!target) {
-          ret = -ESRCH;
-          goto out;
-     }
+     if (pid && pid != current->pid) {
+	     target = find_task_by_pid(pid);
+	     if (!target) {
+	          ret = -ESRCH;
+	          goto out;
+	     }
+     } else
+	     target = current;
 
      data.permitted = cap_t(target->cap_permitted);
      data.inheritable = cap_t(target->cap_inheritable);
author
#6 lore raw

PATCH 1/1 remove duplicated assignment from sys_capget.

Quoted 1 line · * Chris Wright ([email protected]) wrote:

I'll follow up with the patch that removes the duplicate code.

This patch, relative to the last patch in this thread, removes the code from cap_sysget that fills out the capability set being returned to userspace. The module handles this in a policy specific way. This updates the dummy.c module to fill in return data according to superuser policy, and also disables setting capabilities in superuser policy.

[PATCH] remove duplicated assignment from sys_capget.

===== kernel/capability.c 1.6 vs edited =====

kernel/capability.c +0 -3
@@ -63,9 +63,6 @@
      } else
 	     target = current;
 
-     data.permitted = cap_t(target->cap_permitted);
-     data.inheritable = cap_t(target->cap_inheritable); 
-     data.effective = cap_t(target->cap_effective);
      ret = security_ops->capget(target, &data.effective, &data.inheritable, &data.permitted);
 
 out:

===== security/dummy.c 1.4 vs edited =====

security/dummy.c +12 -1
@@ -27,6 +27,17 @@
 static int dummy_capget (struct task_struct *target, kernel_cap_t * effective,
 			 kernel_cap_t * inheritable, kernel_cap_t * permitted)
 {
+	*effective = *inheritable = *permitted = 0;
+	if (!issecure(SECURE_NOROOT)) {
+		if (target->euid == 0) {
+			*permitted |= (~0 & ~CAP_FS_MASK);
+			*effective |= (~0 & ~CAP_TO_MASK(CAP_SETPCAP) & ~CAP_FS_MASK);
+		}
+		if (target->fsuid == 0) {
+			*permitted |= CAP_FS_MASK;
+			*effective |= CAP_FS_MASK;
+		}
+	}
 	return 0;
 }
 

@@ -35,7 +46,7 @@
 			       kernel_cap_t * inheritable,
 			       kernel_cap_t * permitted)
 {
-	return 0;
+	return -EPERM;
 }
 
 static void dummy_capset_set (struct task_struct *target,