2012-10-02 20:30:31

by Andrei Vagin

[permalink] [raw]
Subject: [PATCH] proc: don't show nonexistent capabilities

Without this patch it is really hard to interpret a bounding set,
if CAP_LAST_CAP is unknown for a current kernel.

Non-existant capabilities can not be deleted from a bounding set
with help of prctl.

E.g.: Here are two examples without/with this patch.
CapBnd: ffffffe0fdecffff
CapBnd: 00000000fdecffff

I suggest to hide non-existent capabilities. Here is two reasons.
* It's logically and easier for using.
* It helps to checkpoint-restore capabilities of tasks, because tasks
can be restored on another kernel, where CAP_LAST_CAP is bigger.

Cc: Serge Hallyn <[email protected]>
Cc: Pavel Emelyanov <[email protected]>
Signed-off-by: Andrew Vagin <[email protected]>
---
include/linux/capability.h | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/linux/capability.h b/include/linux/capability.h
index d10b7ed..1642778 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -420,7 +420,8 @@ extern const kernel_cap_t __cap_init_eff_set;
#else /* HAND-CODED capability initializers */

# define CAP_EMPTY_SET ((kernel_cap_t){{ 0, 0 }})
-# define CAP_FULL_SET ((kernel_cap_t){{ ~0, ~0 }})
+# define CAP_FULL_SET ((kernel_cap_t){{ ~0, \
+ CAP_TO_MASK(CAP_LAST_CAP + 1) - 1 } })
# define CAP_FS_SET ((kernel_cap_t){{ CAP_FS_MASK_B0 \
| CAP_TO_MASK(CAP_LINUX_IMMUTABLE), \
CAP_FS_MASK_B1 } })
--
1.7.1


2012-10-03 16:22:35

by Serge E. Hallyn

[permalink] [raw]
Subject: Re: [PATCH] proc: don't show nonexistent capabilities

Quoting Andrew Vagin ([email protected]):
> Without this patch it is really hard to interpret a bounding set,
> if CAP_LAST_CAP is unknown for a current kernel.
>
> Non-existant capabilities can not be deleted from a bounding set
> with help of prctl.
>
> E.g.: Here are two examples without/with this patch.
> CapBnd: ffffffe0fdecffff
> CapBnd: 00000000fdecffff
>
> I suggest to hide non-existent capabilities. Here is two reasons.
> * It's logically and easier for using.
> * It helps to checkpoint-restore capabilities of tasks, because tasks
> can be restored on another kernel, where CAP_LAST_CAP is bigger.
>
> Cc: Serge Hallyn <[email protected]>

Hm, I don't object to this patch. Sounds useful indeed. I can't
help shake the feeling though that something somewhere will get
confused by this (though it shouldn't), so I'd like to do some
testing. Have you run ltp against this? Are you running this
daily with your distro?

> Cc: Pavel Emelyanov <[email protected]>
> Signed-off-by: Andrew Vagin <[email protected]>
> ---
> include/linux/capability.h | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index d10b7ed..1642778 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -420,7 +420,8 @@ extern const kernel_cap_t __cap_init_eff_set;
> #else /* HAND-CODED capability initializers */
>
> # define CAP_EMPTY_SET ((kernel_cap_t){{ 0, 0 }})
> -# define CAP_FULL_SET ((kernel_cap_t){{ ~0, ~0 }})
> +# define CAP_FULL_SET ((kernel_cap_t){{ ~0, \
> + CAP_TO_MASK(CAP_LAST_CAP + 1) - 1 } })
> # define CAP_FS_SET ((kernel_cap_t){{ CAP_FS_MASK_B0 \
> | CAP_TO_MASK(CAP_LINUX_IMMUTABLE), \
> CAP_FS_MASK_B1 } })
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2012-10-04 21:42:28

by Andrei Vagin

[permalink] [raw]
Subject: Re: [PATCH] proc: don't show nonexistent capabilities

2012/10/3 Serge E. Hallyn <[email protected]>:
> Quoting Andrew Vagin ([email protected]):
>> Without this patch it is really hard to interpret a bounding set,
>> if CAP_LAST_CAP is unknown for a current kernel.
>>
>> Non-existant capabilities can not be deleted from a bounding set
>> with help of prctl.
>>
>> E.g.: Here are two examples without/with this patch.
>> CapBnd: ffffffe0fdecffff
>> CapBnd: 00000000fdecffff
>>
>> I suggest to hide non-existent capabilities. Here is two reasons.
>> * It's logically and easier for using.
>> * It helps to checkpoint-restore capabilities of tasks, because tasks
>> can be restored on another kernel, where CAP_LAST_CAP is bigger.
>>
>> Cc: Serge Hallyn <[email protected]>
>
> Hm, I don't object to this patch. Sounds useful indeed. I can't
> help shake the feeling though that something somewhere will get
> confused by this (though it shouldn't), so I'd like to do some
> testing. Have you run ltp against this? Are you running this
> daily with your distro?

I've been using a kernel with this patch on my workstation (FC17) for two days.
The same kernel works on a test server (RHEL6).

I've executed LTP on the kernel with this patch and without it and the
results are not differ.
The results for both kernels are attached.

Thanks.

>
>> Cc: Pavel Emelyanov <[email protected]>
>> Signed-off-by: Andrew Vagin <[email protected]>
>> ---
>> include/linux/capability.h | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/capability.h b/include/linux/capability.h
>> index d10b7ed..1642778 100644
>> --- a/include/linux/capability.h
>> +++ b/include/linux/capability.h
>> @@ -420,7 +420,8 @@ extern const kernel_cap_t __cap_init_eff_set;
>> #else /* HAND-CODED capability initializers */
>>
>> # define CAP_EMPTY_SET ((kernel_cap_t){{ 0, 0 }})
>> -# define CAP_FULL_SET ((kernel_cap_t){{ ~0, ~0 }})
>> +# define CAP_FULL_SET ((kernel_cap_t){{ ~0, \
>> + CAP_TO_MASK(CAP_LAST_CAP + 1) - 1 } })
>> # define CAP_FS_SET ((kernel_cap_t){{ CAP_FS_MASK_B0 \
>> | CAP_TO_MASK(CAP_LINUX_IMMUTABLE), \
>> CAP_FS_MASK_B1 } })
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/


Attachments:
results-3.5.4-1.fc17.x86_64.gz (45.86 kB)
results-3.6.0+-with-patch.gz (45.88 kB)
Download all attachments

2012-10-05 14:03:24

by Serge E. Hallyn

[permalink] [raw]
Subject: Re: [PATCH] proc: don't show nonexistent capabilities

Quoting Andrew Vagin ([email protected]):
> Without this patch it is really hard to interpret a bounding set,
> if CAP_LAST_CAP is unknown for a current kernel.

To be clear, note that you *can* figure it out using
prctl(PR_CAPBSET_DROP). But this is a nice improvement.

> Non-existant capabilities can not be deleted from a bounding set
> with help of prctl.
>
> E.g.: Here are two examples without/with this patch.
> CapBnd: ffffffe0fdecffff
> CapBnd: 00000000fdecffff
>
> I suggest to hide non-existent capabilities. Here is two reasons.
> * It's logically and easier for using.
> * It helps to checkpoint-restore capabilities of tasks, because tasks
> can be restored on another kernel, where CAP_LAST_CAP is bigger.
>
> Cc: Serge Hallyn <[email protected]>

Thanks, Andrew. I saw your other email about having run LTP, I didn't
see any problems from userspace either. Great idea!

Reviewed-by: Serge Hallyn <[email protected]>

Still it's been like that for so long that, just to be safe, I'm cc:ing
Andrew Morgan - can you see any problems with this?

> Cc: Pavel Emelyanov <[email protected]>
> Signed-off-by: Andrew Vagin <[email protected]>
> ---
> include/linux/capability.h | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index d10b7ed..1642778 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -420,7 +420,8 @@ extern const kernel_cap_t __cap_init_eff_set;
> #else /* HAND-CODED capability initializers */
>
> # define CAP_EMPTY_SET ((kernel_cap_t){{ 0, 0 }})
> -# define CAP_FULL_SET ((kernel_cap_t){{ ~0, ~0 }})
> +# define CAP_FULL_SET ((kernel_cap_t){{ ~0, \
> + CAP_TO_MASK(CAP_LAST_CAP + 1) - 1 } })
> # define CAP_FS_SET ((kernel_cap_t){{ CAP_FS_MASK_B0 \
> | CAP_TO_MASK(CAP_LINUX_IMMUTABLE), \
> CAP_FS_MASK_B1 } })
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2012-10-05 15:54:55

by Andrew G. Morgan

[permalink] [raw]
Subject: Re: [PATCH] proc: don't show nonexistent capabilities

I like the sentiment but have you considered the implications for a
default system root user trying to set all=eip ? Existing code can do
this because all bits are accessible by default. If you set the
bounding set to something less than ~0, then any code that currently
does this will start to fail in the common case.

Cheers

Andrew

On Oct 5, 2012 7:13 AM, "Serge E. Hallyn" <[email protected]> wrote:
>
> Quoting Andrew Vagin ([email protected]):
> > Without this patch it is really hard to interpret a bounding set,
> > if CAP_LAST_CAP is unknown for a current kernel.
>
> To be clear, note that you *can* figure it out using
> prctl(PR_CAPBSET_DROP). But this is a nice improvement.
>
> > Non-existant capabilities can not be deleted from a bounding set
> > with help of prctl.
> >
> > E.g.: Here are two examples without/with this patch.
> > CapBnd: ffffffe0fdecffff
> > CapBnd: 00000000fdecffff
> >
> > I suggest to hide non-existent capabilities. Here is two reasons.
> > * It's logically and easier for using.
> > * It helps to checkpoint-restore capabilities of tasks, because tasks
> > can be restored on another kernel, where CAP_LAST_CAP is bigger.
> >
> > Cc: Serge Hallyn <[email protected]>
>
> Thanks, Andrew. I saw your other email about having run LTP, I didn't
> see any problems from userspace either. Great idea!
>
> Reviewed-by: Serge Hallyn <[email protected]>
>
> Still it's been like that for so long that, just to be safe, I'm cc:ing
> Andrew Morgan - can you see any problems with this?
>
> > Cc: Pavel Emelyanov <[email protected]>
> > Signed-off-by: Andrew Vagin <[email protected]>
> > ---
> > include/linux/capability.h | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > index d10b7ed..1642778 100644
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -420,7 +420,8 @@ extern const kernel_cap_t __cap_init_eff_set;
> > #else /* HAND-CODED capability initializers */
> >
> > # define CAP_EMPTY_SET ((kernel_cap_t){{ 0, 0 }})
> > -# define CAP_FULL_SET ((kernel_cap_t){{ ~0, ~0 }})
> > +# define CAP_FULL_SET ((kernel_cap_t){{ ~0, \
> > + CAP_TO_MASK(CAP_LAST_CAP + 1) - 1 } })
> > # define CAP_FS_SET ((kernel_cap_t){{ CAP_FS_MASK_B0 \
> > | CAP_TO_MASK(CAP_LINUX_IMMUTABLE), \
> > CAP_FS_MASK_B1 } })
> > --
> > 1.7.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/

2012-10-05 16:47:03

by Serge Hallyn

[permalink] [raw]
Subject: Re: [PATCH] proc: don't show nonexistent capabilities

Drat, thanks Andrew, I thought I had a testcase for that in LTP, but
apparently not.

capsh --caps="all=eip" -- -c /bin/bash

indeed fails with this patch (and succeeds without).

So

Nacked-by: Serge Hallyn <[email protected]>

since this is a much more common idiom, enough so that I'm not willing
to say userspace should work around it (which indeed it could).

Note that /proc/self/status is a slow path anyway, so updating that
file to output only valid capabilities might be a workable alternative.

-serge

Quoting Andrew G. Morgan ([email protected]):
> I like the sentiment but have you considered the implications for a
> default system root user trying to set all=eip ? Existing code can do
> this because all bits are accessible by default. If you set the
> bounding set to something less than ~0, then any code that currently
> does this will start to fail in the common case.
>
> Cheers
>
> Andrew
>
> On Oct 5, 2012 7:13 AM, "Serge E. Hallyn" <[email protected]> wrote:
> >
> > Quoting Andrew Vagin ([email protected]):
> > > Without this patch it is really hard to interpret a bounding set,
> > > if CAP_LAST_CAP is unknown for a current kernel.
> >
> > To be clear, note that you *can* figure it out using
> > prctl(PR_CAPBSET_DROP). But this is a nice improvement.
> >
> > > Non-existant capabilities can not be deleted from a bounding set
> > > with help of prctl.
> > >
> > > E.g.: Here are two examples without/with this patch.
> > > CapBnd: ffffffe0fdecffff
> > > CapBnd: 00000000fdecffff
> > >
> > > I suggest to hide non-existent capabilities. Here is two reasons.
> > > * It's logically and easier for using.
> > > * It helps to checkpoint-restore capabilities of tasks, because tasks
> > > can be restored on another kernel, where CAP_LAST_CAP is bigger.
> > >
> > > Cc: Serge Hallyn <[email protected]>
> >
> > Thanks, Andrew. I saw your other email about having run LTP, I didn't
> > see any problems from userspace either. Great idea!
> >
> > Reviewed-by: Serge Hallyn <[email protected]>
> >
> > Still it's been like that for so long that, just to be safe, I'm cc:ing
> > Andrew Morgan - can you see any problems with this?
> >
> > > Cc: Pavel Emelyanov <[email protected]>
> > > Signed-off-by: Andrew Vagin <[email protected]>
> > > ---
> > > include/linux/capability.h | 3 ++-
> > > 1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > > index d10b7ed..1642778 100644
> > > --- a/include/linux/capability.h
> > > +++ b/include/linux/capability.h
> > > @@ -420,7 +420,8 @@ extern const kernel_cap_t __cap_init_eff_set;
> > > #else /* HAND-CODED capability initializers */
> > >
> > > # define CAP_EMPTY_SET ((kernel_cap_t){{ 0, 0 }})
> > > -# define CAP_FULL_SET ((kernel_cap_t){{ ~0, ~0 }})
> > > +# define CAP_FULL_SET ((kernel_cap_t){{ ~0, \
> > > + CAP_TO_MASK(CAP_LAST_CAP + 1) - 1 } })
> > > # define CAP_FS_SET ((kernel_cap_t){{ CAP_FS_MASK_B0 \
> > > | CAP_TO_MASK(CAP_LINUX_IMMUTABLE), \
> > > CAP_FS_MASK_B1 } })
> > > --
> > > 1.7.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/