Capabilities-related change in 2.5.40

Capabilities-related change in 2.5.40

[Daniel Jacobowitz]

First of all, I think the LSM code is confused in its use of cap_t.  I think
that cap_capget should be using to_cap_t instead; it's converting _to_ a
kernel_cap_t, right?



Second of all, my login shell (as a user) gets a very bizarre response to sys_capget:

capget(0x19980330, 0, {CAP_CHOWN | CAP_DAC_OVERRIDE | CAP_DAC_READ_SEARCH |
  CAP_FOWNER | CAP_FSETID | CAP_KILL | CAP_SETGID | CAP_SETUID |
  CAP_LINUX_IMMUTABLE | CAP_NET_BIND_SERVICE | CAP_NET_BROADCAST |
  CAP_NET_ADMIN | CAP_NET_RAW | CAP_IPC_LOCK | CAP_IPC_OWNER | CAP_SYS_MODULE
  | CAP_SYS_RAWIO | CAP_SYS_CHROOT | CAP_SYS_PTRACE | CAP_SYS_PACCT |
  CAP_SYS_ADMIN | CAP_SYS_BOOT | CAP_SYS_NICE | CAP_SYS_RESOURCE |
  CAP_SYS_TIME | CAP_SYS_TTY_CONFIG | 0xf8000000,
  CAP_CHOWN | CAP_DAC_OVERRIDE
  | CAP_DAC_READ_SEARCH | CAP_FOWNER | CAP_FSETID | CAP_KILL | CAP_SETGID |
  CAP_SETUID | CAP_SETPCAP | CAP_LINUX_IMMUTABLE | CAP_NET_BIND_SERVICE |
  CAP_NET_BROADCAST | CAP_NET_ADMIN | CAP_NET_RAW | CAP_IPC_LOCK |
  CAP_IPC_OWNER | CAP_SYS_MODULE | CAP_SYS_RAWIO | CAP_SYS_CHROOT |
  CAP_SYS_PTRACE | CAP_SYS_PACCT | CAP_SYS_ADMIN | CAP_SYS_BOOT | CAP_SYS_NICE
  | CAP_SYS_RESOURCE | CAP_SYS_TIME | CAP_SYS_TTY_CONFIG | 0xf8000000,}) = 0

The reason?  cap_get_proc has always been broken.  But the capability set of
task 0, swapper, has now changed.  It used to be empty.  So, I'll go report
this to libcap.  The change in capabilities for swapper is presumably
benign.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: Capabilities-related change in 2.5.40

[Chris Wright]

* Daniel Jacobowitz (dan@debian.org) wrote:
> First of all, I think the LSM code is confused in its use of cap_t.  I think
> that cap_capget should be using to_cap_t instead; it's converting _to_ a
> kernel_cap_t, right?

I believe capget should be using cap_t (it's been this way since
2.2).  The macros are kind of meaningless unless you have defined
STRICT_CAP_T_TYPECHECKS.  And anyway, cap_t is used to extract a cap
(to a __u32) from a structure.  While to_cap_t is used to place a __u32
in a cap structure.  capget is retrieving the value from a structure and
simply placing it in a __u32 that is copied back to userspace.  However,
there is a merge error there with duplicate code.

--- 1.5/kernel/capability.c	Sun Sep 15 12:19:29 2002
+++ edited/capability.c	Tue Oct  1 13:30:19 2002
@@ -60,9 +60,6 @@
           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:

> Second of all, my login shell (as a user) gets a very bizarre response to sys_capget:
> 
> capget(0x19980330, 0, {CAP_CHOWN | CAP_DAC_OVERRIDE | CAP_DAC_READ_SEARCH |
>   CAP_FOWNER | CAP_FSETID | CAP_KILL | CAP_SETGID | CAP_SETUID |
>   CAP_LINUX_IMMUTABLE | CAP_NET_BIND_SERVICE | CAP_NET_BROADCAST |
>   CAP_NET_ADMIN | CAP_NET_RAW | CAP_IPC_LOCK | CAP_IPC_OWNER | CAP_SYS_MODULE
>   | CAP_SYS_RAWIO | CAP_SYS_CHROOT | CAP_SYS_PTRACE | CAP_SYS_PACCT |
>   CAP_SYS_ADMIN | CAP_SYS_BOOT | CAP_SYS_NICE | CAP_SYS_RESOURCE |
>   CAP_SYS_TIME | CAP_SYS_TTY_CONFIG | 0xf8000000,
>   CAP_CHOWN | CAP_DAC_OVERRIDE
>   | CAP_DAC_READ_SEARCH | CAP_FOWNER | CAP_FSETID | CAP_KILL | CAP_SETGID |
>   CAP_SETUID | CAP_SETPCAP | CAP_LINUX_IMMUTABLE | CAP_NET_BIND_SERVICE |
>   CAP_NET_BROADCAST | CAP_NET_ADMIN | CAP_NET_RAW | CAP_IPC_LOCK |
>   CAP_IPC_OWNER | CAP_SYS_MODULE | CAP_SYS_RAWIO | CAP_SYS_CHROOT |
>   CAP_SYS_PTRACE | CAP_SYS_PACCT | CAP_SYS_ADMIN | CAP_SYS_BOOT | CAP_SYS_NICE
>   | CAP_SYS_RESOURCE | CAP_SYS_TIME | CAP_SYS_TTY_CONFIG | 0xf8000000,}) = 0
> 
> The reason?  cap_get_proc has always been broken.  But the capability set of
> task 0, swapper, has now changed.  It used to be empty.  So, I'll go report
> this to libcap.  The change in capabilities for swapper is presumably
> benign.

I'm not sure what you are pointing out?  Is cap_get_proc using header.pid = 0
regardless?  Hmm, yes.

cap_get_proc()
	cap_init()
		malloc
		memset(0)
		head.version = _LINUX_CAPABILITY_VERSION
	capget(&head, &set)	<-- head.pid = 0


Also, I've just looked at 2.2, 2.4, 2.5 and they all have the same caps
for INIT_TASK:

2.2
#define CAP_FULL_SET        to_cap_t(~0)
#define CAP_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
#define CAP_INIT_INH_SET    to_cap_t(0)
/* caps */	CAP_INIT_EFF_SET,CAP_INIT_INH_SET,CAP_FULL_SET,	\
/* keep_caps */	0,  						\

2.4
#define CAP_FULL_SET        to_cap_t(~0)
#define CAP_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
#define CAP_INIT_INH_SET    to_cap_t(0)
    cap_effective:	CAP_INIT_EFF_SET,	\
    cap_inheritable:	CAP_INIT_INH_SET,	\
    cap_permitted:	CAP_FULL_SET,		\
    keep_capabilities:	0,			\

2.5
#define CAP_FULL_SET        to_cap_t(~0)
#define CAP_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
#define CAP_INIT_INH_SET    to_cap_t(0)
	.cap_effective  = CAP_INIT_EFF_SET,	\
	.cap_inheritable = CAP_INIT_INH_SET,	\
	.cap_permitted  = CAP_FULL_SET,		\
	.keep_capabilities = 0,			\

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: Capabilities-related change in 2.5.40

[Daniel Jacobowitz]

On Tue, Oct 01, 2002 at 01:45:52PM -0700, Chris Wright wrote:
> > Second of all, my login shell (as a user) gets a very bizarre response to sys_capget:
> > 
> > capget(0x19980330, 0, {CAP_CHOWN | CAP_DAC_OVERRIDE | CAP_DAC_READ_SEARCH |
> >   CAP_FOWNER | CAP_FSETID | CAP_KILL | CAP_SETGID | CAP_SETUID |
> >   CAP_LINUX_IMMUTABLE | CAP_NET_BIND_SERVICE | CAP_NET_BROADCAST |
> >   CAP_NET_ADMIN | CAP_NET_RAW | CAP_IPC_LOCK | CAP_IPC_OWNER | CAP_SYS_MODULE
> >   | CAP_SYS_RAWIO | CAP_SYS_CHROOT | CAP_SYS_PTRACE | CAP_SYS_PACCT |
> >   CAP_SYS_ADMIN | CAP_SYS_BOOT | CAP_SYS_NICE | CAP_SYS_RESOURCE |
> >   CAP_SYS_TIME | CAP_SYS_TTY_CONFIG | 0xf8000000,
> >   CAP_CHOWN | CAP_DAC_OVERRIDE
> >   | CAP_DAC_READ_SEARCH | CAP_FOWNER | CAP_FSETID | CAP_KILL | CAP_SETGID |
> >   CAP_SETUID | CAP_SETPCAP | CAP_LINUX_IMMUTABLE | CAP_NET_BIND_SERVICE |
> >   CAP_NET_BROADCAST | CAP_NET_ADMIN | CAP_NET_RAW | CAP_IPC_LOCK |
> >   CAP_IPC_OWNER | CAP_SYS_MODULE | CAP_SYS_RAWIO | CAP_SYS_CHROOT |
> >   CAP_SYS_PTRACE | CAP_SYS_PACCT | CAP_SYS_ADMIN | CAP_SYS_BOOT | CAP_SYS_NICE
> >   | CAP_SYS_RESOURCE | CAP_SYS_TIME | CAP_SYS_TTY_CONFIG | 0xf8000000,}) = 0
> > 
> > The reason?  cap_get_proc has always been broken.  But the capability set of
> > task 0, swapper, has now changed.  It used to be empty.  So, I'll go report
> > this to libcap.  The change in capabilities for swapper is presumably
> > benign.
> 
> I'm not sure what you are pointing out?  Is cap_get_proc using header.pid = 0
> regardless?  Hmm, yes.
> 
> cap_get_proc()
> 	cap_init()
> 		malloc
> 		memset(0)
> 		head.version = _LINUX_CAPABILITY_VERSION
> 	capget(&head, &set)	<-- head.pid = 0
> 

Yes.  It was pointed out to me that libcap2 snapshots behave correctly.

> Also, I've just looked at 2.2, 2.4, 2.5 and they all have the same caps
> for INIT_TASK:
> 
> 2.2
> #define CAP_FULL_SET        to_cap_t(~0)
> #define CAP_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
> #define CAP_INIT_INH_SET    to_cap_t(0)
> /* caps */	CAP_INIT_EFF_SET,CAP_INIT_INH_SET,CAP_FULL_SET,	\
> /* keep_caps */	0,  						\
> 
> 2.4
> #define CAP_FULL_SET        to_cap_t(~0)
> #define CAP_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
> #define CAP_INIT_INH_SET    to_cap_t(0)
>     cap_effective:	CAP_INIT_EFF_SET,	\
>     cap_inheritable:	CAP_INIT_INH_SET,	\
>     cap_permitted:	CAP_FULL_SET,		\
>     keep_capabilities:	0,			\
> 
> 2.5
> #define CAP_FULL_SET        to_cap_t(~0)
> #define CAP_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
> #define CAP_INIT_INH_SET    to_cap_t(0)
> 	.cap_effective  = CAP_INIT_EFF_SET,	\
> 	.cap_inheritable = CAP_INIT_INH_SET,	\
> 	.cap_permitted  = CAP_FULL_SET,		\
> 	.keep_capabilities = 0,			\

Not init: swapper.  Try it on 2.4:
drow@nevyn:~% getpcaps 0
Capabilities for `0': =

2.5.40 gives me a very different answer :)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

[PATCH] Re: Capabilities-related change in 2.5.40

[Chris Wright]

* Daniel Jacobowitz (dan@debian.org) wrote:
> 
> Yes.  It was pointed out to me that libcap2 snapshots behave correctly.

Ah, thanks for the info.  Hmm, libcap2 still looks like it sets up the
header with pid == 0.  Maybe I'm missing something.

> Not init: swapper.

Yes, although INIT_TASK sets up the task_struct for swapper.

> Try it on 2.4:
> drow@nevyn:~% getpcaps 0
> Capabilities for `0': =
> 
> 2.5.40 gives me a very different answer :)

Heh, you're right.  However, 2.5.20 behaves the same as 2.4.  Looking
back this appears to be caused by 2.5.21 locking cleanups done by rml.
The older code interpreted pid == 0 to mean current, whereas the new
code unconditionally does find_task_by_pid(0).  This patch fixes that,
and then pid == 0 from libcap should work again.

--- 1.5/kernel/capability.c	Sun Sep 15 12:19:29 2002
+++ edited/kernel/capability.c	Wed Oct  2 00:28:32 2002
@@ -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))

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Re: Capabilities-related change in 2.5.40

[Daniel Jacobowitz]

On Wed, Oct 02, 2002 at 12:38:17AM -0700, Chris Wright wrote:
> * Daniel Jacobowitz (dan@debian.org) wrote:
> > 
> > Yes.  It was pointed out to me that libcap2 snapshots behave correctly.
> 
> Ah, thanks for the info.  Hmm, libcap2 still looks like it sets up the
> header with pid == 0.  Maybe I'm missing something.

Look at cap_proc.c:_libcap_cappid :

#if (_LINUX_CAPABILITY_VERSION == 0x19980330)

    if (_libcap_kernel_version == 0x19980330) {
        cap_d->head.pid = pid;
    }
    _cap_debug("pid: %d\n", cap_d->head.pid);

> 
> > Not init: swapper.
> 
> Yes, although INIT_TASK sets up the task_struct for swapper.

Oh, right.

> > Try it on 2.4:
> > drow@nevyn:~% getpcaps 0
> > Capabilities for `0': =
> > 
> > 2.5.40 gives me a very different answer :)
> 
> Heh, you're right.  However, 2.5.20 behaves the same as 2.4.  Looking
> back this appears to be caused by 2.5.21 locking cleanups done by rml.
> The older code interpreted pid == 0 to mean current, whereas the new
> code unconditionally does find_task_by_pid(0).  This patch fixes that,
> and then pid == 0 from libcap should work again.

How very odd.  I have been running 2.5 on that machine for a while, and
the bug only showed up somewhere between 2.5.36 and 2.5.40.  Maybe a
coincidence triggered by the PID hashing... your tabbing is a little
odd but the patch looks right to me.  Thanks!

> 
> --- 1.5/kernel/capability.c	Sun Sep 15 12:19:29 2002
> +++ edited/kernel/capability.c	Wed Oct  2 00:28:32 2002
> @@ -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))
> 
> thanks,
> -chris
> -- 
> Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net
> -
> 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/
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH] Re: Capabilities-related change in 2.5.40

[Robert Love]

On Wed, 2002-10-02 at 09:23, Daniel Jacobowitz wrote:

> How very odd.  I have been running 2.5 on that machine for a while, and
> the bug only showed up somewhere between 2.5.36 and 2.5.40.  Maybe a
> coincidence triggered by the PID hashing... your tabbing is a little
> odd but the patch looks right to me.  Thanks!

I too wonder if the bug is due to the pid changes and not me :)

find_task_by_pid(0) should just return current, which was my intention
to avoid conditional code-paths.  Maybe I should of used
find_process_by_pid()... it seems in the latest 2.5 that still returns
current if !pid, at least.

	Robert Love

Re: [PATCH] Re: Capabilities-related change in 2.5.40

[Chris Wright]

* Daniel Jacobowitz (dan@debian.org) wrote:
> 
> Look at cap_proc.c:_libcap_cappid :

Right, but cap_get_proc calls _libcap_cappid with pid of 0.  At any
rate, I believe pid == 0 is intentional to pick up the current
capabilities.

> How very odd.  I have been running 2.5 on that machine for a while, and
> the bug only showed up somewhere between 2.5.36 and 2.5.40.  Maybe a
> coincidence triggered by the PID hashing... your tabbing is a little
> odd but the patch looks right to me.  Thanks!

I tried on various older 2.5 kernels (> 2.5.20) and they returned -ESRCH.
I agree, the PID hashing probably started picking up swapper.  And yes
the tabbing is odd.  The file is badly in need of Lindent, as it's mostly
space tabbed.  I didn't want to hide the patch in whitespace changes ;-)

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Re: Capabilities-related change in 2.5.40

[Chris Wright]

* Robert Love (rml@tech9.net) wrote:
> On Wed, 2002-10-02 at 09:23, Daniel Jacobowitz wrote:
> 
> > How very odd.  I have been running 2.5 on that machine for a while, and
> > the bug only showed up somewhere between 2.5.36 and 2.5.40.  Maybe a
> > coincidence triggered by the PID hashing... your tabbing is a little
> > odd but the patch looks right to me.  Thanks!
> 
> I too wonder if the bug is due to the pid changes and not me :)

No way man! ;-)

> find_task_by_pid(0) should just return current, which was my intention
> to avoid conditional code-paths.  Maybe I should of used
> find_process_by_pid()... it seems in the latest 2.5 that still returns
> current if !pid, at least.

kernel/sched.c::static inline task_t *find_process_by_pid...

Guess that won't work w/out more changes.  Perhaps it's simpler/safer
to be just be explicit in this case.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Re: Capabilities-related change in 2.5.40

[Robert Love]

On Wed, 2002-10-02 at 12:44, Chris Wright wrote:

> kernel/sched.c::static inline task_t *find_process_by_pid...
> 
> Guess that won't work w/out more changes.  Perhaps it's simpler/safer
> to be just be explicit in this case.

>From 2.5.40:

        static inline task_t *find_process_by_pid(pid_t pid)
        {
        	return pid ? find_task_by_pid(pid) : current;
        }

should work :)

	Robert Love

Re: [PATCH] Re: Capabilities-related change in 2.5.40

[Chris Wright]

* Robert Love (rml@tech9.net) wrote:
> On Wed, 2002-10-02 at 12:44, Chris Wright wrote:
> 
> > kernel/sched.c::static inline task_t *find_process_by_pid...
> > 
> > Guess that won't work w/out more changes.  Perhaps it's simpler/safer
> > to be just be explicit in this case.
> 
> >From 2.5.40:
> 
>         static inline task_t *find_process_by_pid(pid_t pid)
>         {
>         	return pid ? find_task_by_pid(pid) : current;
>         }
> 
> should work :)

heh, sorry, I meant the static inline part.
cheers,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net