diff mbox

RFC: capabilities(7): notes for kernel developers

Message ID 20161216201026.GB14856@mail.hallyn.com (mailing list archive)
State New, archived
Headers show

Commit Message

Serge Hallyn Dec. 16, 2016, 8:10 p.m. UTC
Quoting Michael Kerrisk (man-pages) (mtk.manpages@gmail.com):
> On 12/16/2016 01:44 AM, Casey Schaufler wrote:
> > On 12/15/2016 4:31 PM, John Stultz wrote:
> >> On Thu, Dec 15, 2016 at 12:40 PM, Casey Schaufler
> >> <casey@schaufler-ca.com> wrote:
> >>> On 12/15/2016 11:41 AM, Michael Kerrisk (man-pages) wrote:
> >>>> On 12/15/2016 05:29 PM, Casey Schaufler wrote:
> >>>>> CAP_WAKE_ALARM could readily be CAP_TIME.
> >>>> Actually, I don't quite understand what you mean with that sentence.
> >>>> Could you elaborate?
> >>> Should have said CAP_SYS_TIME
> >>>
> >>> Setting an alarm could be considered a time management function,
> >>> depending on what it actually does.
> >> Just a nit here. CAP_WAKE_ALARM is more about the privilege of waking
> >> a system from suspend, while CAP_SYS_TIME covers the ability to set
> >> the time. One wouldn't necessarily want to give applications which
> >> could wake a system up the capability to also set the time.
> > 
> > Doesn't really matter, except that an ignorant developer
> > might make the mistake I did and assume that WAKE_ALARM
> > was somehow related to time management. If you want to use
> > it as an example don't let my dunderheadedness get in your
> > way.
> 
> Actually, I decided it wasn't such a good example anyway.
> That capability could potentially be generic. (But it probably
> should better have been named something like 'CAP_WAKE_SYSTEM'.)

How about:

Subject: [PATCH 1/1] capabilities: alias CAP_WAKE_SYSTEM to CAP_WAKE_ALARM

As suggested by Michael Kerrisk his is a less confusing name, and
this won't break any old userspace.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
---
 include/uapi/linux/capability.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

John Stultz Dec. 16, 2016, 8:20 p.m. UTC | #1
On Fri, Dec 16, 2016 at 12:10 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Michael Kerrisk (man-pages) (mtk.manpages@gmail.com):
>> On 12/16/2016 01:44 AM, Casey Schaufler wrote:
>> > On 12/15/2016 4:31 PM, John Stultz wrote:
>> >> On Thu, Dec 15, 2016 at 12:40 PM, Casey Schaufler
>> >> <casey@schaufler-ca.com> wrote:
>> >>> On 12/15/2016 11:41 AM, Michael Kerrisk (man-pages) wrote:
>> >>>> On 12/15/2016 05:29 PM, Casey Schaufler wrote:
>> >>>>> CAP_WAKE_ALARM could readily be CAP_TIME.
>> >>>> Actually, I don't quite understand what you mean with that sentence.
>> >>>> Could you elaborate?
>> >>> Should have said CAP_SYS_TIME
>> >>>
>> >>> Setting an alarm could be considered a time management function,
>> >>> depending on what it actually does.
>> >> Just a nit here. CAP_WAKE_ALARM is more about the privilege of waking
>> >> a system from suspend, while CAP_SYS_TIME covers the ability to set
>> >> the time. One wouldn't necessarily want to give applications which
>> >> could wake a system up the capability to also set the time.
>> >
>> > Doesn't really matter, except that an ignorant developer
>> > might make the mistake I did and assume that WAKE_ALARM
>> > was somehow related to time management. If you want to use
>> > it as an example don't let my dunderheadedness get in your
>> > way.
>>
>> Actually, I decided it wasn't such a good example anyway.
>> That capability could potentially be generic. (But it probably
>> should better have been named something like 'CAP_WAKE_SYSTEM'.)
>
> How about:
>
> Subject: [PATCH 1/1] capabilities: alias CAP_WAKE_SYSTEM to CAP_WAKE_ALARM
>
> As suggested by Michael Kerrisk his is a less confusing name, and
> this won't break any old userspace.
>
> Signed-off-by: Serge Hallyn <serge@hallyn.com>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> ---
>  include/uapi/linux/capability.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index fd4f87d..ba972ff 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -357,6 +357,8 @@ struct vfs_ns_cap_data {
>
>  #define CAP_WAKE_ALARM            35
>
> +#define CAP_WAKE_SYSTEM      CAP_WAKE_ALARM
> +

I was thinking of the same thing. Although I might rename the
numerical define to WAKE_SYSTEM and put WAKE_ALARM as the alias (along
with a comment as to WAKE_ALARM being deprecated), so its more clear
which is the one that ought to be used by new code.

However, in the spirit of this thread, we might even consider
broadening the cap silo a bit further, to something like
CAP_WAKE_SUSPEND, such that it might also be able to cover broader PM
actions?

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Serge Hallyn Dec. 16, 2016, 9:05 p.m. UTC | #2
Quoting John Stultz (john.stultz@linaro.org):
> On Fri, Dec 16, 2016 at 12:10 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting Michael Kerrisk (man-pages) (mtk.manpages@gmail.com):
> >> On 12/16/2016 01:44 AM, Casey Schaufler wrote:
> >> > On 12/15/2016 4:31 PM, John Stultz wrote:
> >> >> On Thu, Dec 15, 2016 at 12:40 PM, Casey Schaufler
> >> >> <casey@schaufler-ca.com> wrote:
> >> >>> On 12/15/2016 11:41 AM, Michael Kerrisk (man-pages) wrote:
> >> >>>> On 12/15/2016 05:29 PM, Casey Schaufler wrote:
> >> >>>>> CAP_WAKE_ALARM could readily be CAP_TIME.
> >> >>>> Actually, I don't quite understand what you mean with that sentence.
> >> >>>> Could you elaborate?
> >> >>> Should have said CAP_SYS_TIME
> >> >>>
> >> >>> Setting an alarm could be considered a time management function,
> >> >>> depending on what it actually does.
> >> >> Just a nit here. CAP_WAKE_ALARM is more about the privilege of waking
> >> >> a system from suspend, while CAP_SYS_TIME covers the ability to set
> >> >> the time. One wouldn't necessarily want to give applications which
> >> >> could wake a system up the capability to also set the time.
> >> >
> >> > Doesn't really matter, except that an ignorant developer
> >> > might make the mistake I did and assume that WAKE_ALARM
> >> > was somehow related to time management. If you want to use
> >> > it as an example don't let my dunderheadedness get in your
> >> > way.
> >>
> >> Actually, I decided it wasn't such a good example anyway.
> >> That capability could potentially be generic. (But it probably
> >> should better have been named something like 'CAP_WAKE_SYSTEM'.)
> >
> > How about:
> >
> > Subject: [PATCH 1/1] capabilities: alias CAP_WAKE_SYSTEM to CAP_WAKE_ALARM
> >
> > As suggested by Michael Kerrisk his is a less confusing name, and
> > this won't break any old userspace.
> >
> > Signed-off-by: Serge Hallyn <serge@hallyn.com>
> > Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> > ---
> >  include/uapi/linux/capability.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> > index fd4f87d..ba972ff 100644
> > --- a/include/uapi/linux/capability.h
> > +++ b/include/uapi/linux/capability.h
> > @@ -357,6 +357,8 @@ struct vfs_ns_cap_data {
> >
> >  #define CAP_WAKE_ALARM            35
> >
> > +#define CAP_WAKE_SYSTEM      CAP_WAKE_ALARM
> > +
> 
> I was thinking of the same thing. Although I might rename the
> numerical define to WAKE_SYSTEM and put WAKE_ALARM as the alias (along
> with a comment as to WAKE_ALARM being deprecated), so its more clear
> which is the one that ought to be used by new code.
> 
> However, in the spirit of this thread, we might even consider
> broadening the cap silo a bit further, to something like
> CAP_WAKE_SUSPEND, such that it might also be able to cover broader PM
> actions?

Or just CAP_UNSUSPEND?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Stultz Dec. 16, 2016, 9:16 p.m. UTC | #3
On Fri, Dec 16, 2016 at 1:05 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting John Stultz (john.stultz@linaro.org):
>> On Fri, Dec 16, 2016 at 12:10 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > Quoting Michael Kerrisk (man-pages) (mtk.manpages@gmail.com):
>> >> On 12/16/2016 01:44 AM, Casey Schaufler wrote:
>> >> > On 12/15/2016 4:31 PM, John Stultz wrote:
>> >> >> On Thu, Dec 15, 2016 at 12:40 PM, Casey Schaufler
>> >> >> <casey@schaufler-ca.com> wrote:
>> >> >>> On 12/15/2016 11:41 AM, Michael Kerrisk (man-pages) wrote:
>> >> >>>> On 12/15/2016 05:29 PM, Casey Schaufler wrote:
>> >> >>>>> CAP_WAKE_ALARM could readily be CAP_TIME.
>> >> >>>> Actually, I don't quite understand what you mean with that sentence.
>> >> >>>> Could you elaborate?
>> >> >>> Should have said CAP_SYS_TIME
>> >> >>>
>> >> >>> Setting an alarm could be considered a time management function,
>> >> >>> depending on what it actually does.
>> >> >> Just a nit here. CAP_WAKE_ALARM is more about the privilege of waking
>> >> >> a system from suspend, while CAP_SYS_TIME covers the ability to set
>> >> >> the time. One wouldn't necessarily want to give applications which
>> >> >> could wake a system up the capability to also set the time.
>> >> >
>> >> > Doesn't really matter, except that an ignorant developer
>> >> > might make the mistake I did and assume that WAKE_ALARM
>> >> > was somehow related to time management. If you want to use
>> >> > it as an example don't let my dunderheadedness get in your
>> >> > way.
>> >>
>> >> Actually, I decided it wasn't such a good example anyway.
>> >> That capability could potentially be generic. (But it probably
>> >> should better have been named something like 'CAP_WAKE_SYSTEM'.)
>> >
>> > How about:
>> >
>> > Subject: [PATCH 1/1] capabilities: alias CAP_WAKE_SYSTEM to CAP_WAKE_ALARM
>> >
>> > As suggested by Michael Kerrisk his is a less confusing name, and
>> > this won't break any old userspace.
>> >
>> > Signed-off-by: Serge Hallyn <serge@hallyn.com>
>> > Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>> > ---
>> >  include/uapi/linux/capability.h | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>> > index fd4f87d..ba972ff 100644
>> > --- a/include/uapi/linux/capability.h
>> > +++ b/include/uapi/linux/capability.h
>> > @@ -357,6 +357,8 @@ struct vfs_ns_cap_data {
>> >
>> >  #define CAP_WAKE_ALARM            35
>> >
>> > +#define CAP_WAKE_SYSTEM      CAP_WAKE_ALARM
>> > +
>>
>> I was thinking of the same thing. Although I might rename the
>> numerical define to WAKE_SYSTEM and put WAKE_ALARM as the alias (along
>> with a comment as to WAKE_ALARM being deprecated), so its more clear
>> which is the one that ought to be used by new code.
>>
>> However, in the spirit of this thread, we might even consider
>> broadening the cap silo a bit further, to something like
>> CAP_WAKE_SUSPEND, such that it might also be able to cover broader PM
>> actions?
>
> Or just CAP_UNSUSPEND?

I guess I was trying to capture it could be use for actions like both
waking and suspending the system.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Kerrisk (man-pages) Dec. 17, 2016, 9:01 p.m. UTC | #4
On 12/16/2016 09:10 PM, Serge E. Hallyn wrote:
> Quoting Michael Kerrisk (man-pages) (mtk.manpages@gmail.com):
>> On 12/16/2016 01:44 AM, Casey Schaufler wrote:
>>> On 12/15/2016 4:31 PM, John Stultz wrote:
>>>> On Thu, Dec 15, 2016 at 12:40 PM, Casey Schaufler
>>>> <casey@schaufler-ca.com> wrote:
>>>>> On 12/15/2016 11:41 AM, Michael Kerrisk (man-pages) wrote:
>>>>>> On 12/15/2016 05:29 PM, Casey Schaufler wrote:
>>>>>>> CAP_WAKE_ALARM could readily be CAP_TIME.
>>>>>> Actually, I don't quite understand what you mean with that sentence.
>>>>>> Could you elaborate?
>>>>> Should have said CAP_SYS_TIME
>>>>>
>>>>> Setting an alarm could be considered a time management function,
>>>>> depending on what it actually does.
>>>> Just a nit here. CAP_WAKE_ALARM is more about the privilege of waking
>>>> a system from suspend, while CAP_SYS_TIME covers the ability to set
>>>> the time. One wouldn't necessarily want to give applications which
>>>> could wake a system up the capability to also set the time.
>>>
>>> Doesn't really matter, except that an ignorant developer
>>> might make the mistake I did and assume that WAKE_ALARM
>>> was somehow related to time management. If you want to use
>>> it as an example don't let my dunderheadedness get in your
>>> way.
>>
>> Actually, I decided it wasn't such a good example anyway.
>> That capability could potentially be generic. (But it probably
>> should better have been named something like 'CAP_WAKE_SYSTEM'.)
> 
> How about:
> 
> Subject: [PATCH 1/1] capabilities: alias CAP_WAKE_SYSTEM to CAP_WAKE_ALARM
> 
> As suggested by Michael Kerrisk his is a less confusing name, and
> this won't break any old userspace.

Yes, but. The names are used in user-space: should cap_to_text() or
cap_from_text() be reworked to understand these new names? For the former,
it seems like a bad idea, and for the latter it's dubious I would say.
So, I'm concerned that renaming/adding a synonym at the API level 
creates some inconsistency with user-space. Maybe I worry too much?

What certainly would be good is to more clearly document the (broader)
purpose of the (misnamed) capability in capabilities(7). Something along
the lines of John's suggestion.

Cheers,

Michael

> Signed-off-by: Serge Hallyn <serge@hallyn.com>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> ---
>  include/uapi/linux/capability.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index fd4f87d..ba972ff 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -357,6 +357,8 @@ struct vfs_ns_cap_data {
>  
>  #define CAP_WAKE_ALARM            35
>  
> +#define CAP_WAKE_SYSTEM      CAP_WAKE_ALARM
> +
>  /* Allow preventing system suspends */
>  
>  #define CAP_BLOCK_SUSPEND    36
>
Rafael J. Wysocki Dec. 19, 2016, 8:20 p.m. UTC | #5
On Friday, December 16, 2016 01:16:15 PM John Stultz wrote:
> On Fri, Dec 16, 2016 at 1:05 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting John Stultz (john.stultz@linaro.org):
> >> On Fri, Dec 16, 2016 at 12:10 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> >> > Quoting Michael Kerrisk (man-pages) (mtk.manpages@gmail.com):
> >> >> On 12/16/2016 01:44 AM, Casey Schaufler wrote:
> >> >> > On 12/15/2016 4:31 PM, John Stultz wrote:
> >> >> >> On Thu, Dec 15, 2016 at 12:40 PM, Casey Schaufler
> >> >> >> <casey@schaufler-ca.com> wrote:
> >> >> >>> On 12/15/2016 11:41 AM, Michael Kerrisk (man-pages) wrote:
> >> >> >>>> On 12/15/2016 05:29 PM, Casey Schaufler wrote:
> >> >> >>>>> CAP_WAKE_ALARM could readily be CAP_TIME.
> >> >> >>>> Actually, I don't quite understand what you mean with that sentence.
> >> >> >>>> Could you elaborate?
> >> >> >>> Should have said CAP_SYS_TIME
> >> >> >>>
> >> >> >>> Setting an alarm could be considered a time management function,
> >> >> >>> depending on what it actually does.
> >> >> >> Just a nit here. CAP_WAKE_ALARM is more about the privilege of waking
> >> >> >> a system from suspend, while CAP_SYS_TIME covers the ability to set
> >> >> >> the time. One wouldn't necessarily want to give applications which
> >> >> >> could wake a system up the capability to also set the time.
> >> >> >
> >> >> > Doesn't really matter, except that an ignorant developer
> >> >> > might make the mistake I did and assume that WAKE_ALARM
> >> >> > was somehow related to time management. If you want to use
> >> >> > it as an example don't let my dunderheadedness get in your
> >> >> > way.
> >> >>
> >> >> Actually, I decided it wasn't such a good example anyway.
> >> >> That capability could potentially be generic. (But it probably
> >> >> should better have been named something like 'CAP_WAKE_SYSTEM'.)
> >> >
> >> > How about:
> >> >
> >> > Subject: [PATCH 1/1] capabilities: alias CAP_WAKE_SYSTEM to CAP_WAKE_ALARM
> >> >
> >> > As suggested by Michael Kerrisk his is a less confusing name, and
> >> > this won't break any old userspace.
> >> >
> >> > Signed-off-by: Serge Hallyn <serge@hallyn.com>
> >> > Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> >> > ---
> >> >  include/uapi/linux/capability.h | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> >> > index fd4f87d..ba972ff 100644
> >> > --- a/include/uapi/linux/capability.h
> >> > +++ b/include/uapi/linux/capability.h
> >> > @@ -357,6 +357,8 @@ struct vfs_ns_cap_data {
> >> >
> >> >  #define CAP_WAKE_ALARM            35
> >> >
> >> > +#define CAP_WAKE_SYSTEM      CAP_WAKE_ALARM
> >> > +
> >>
> >> I was thinking of the same thing. Although I might rename the
> >> numerical define to WAKE_SYSTEM and put WAKE_ALARM as the alias (along
> >> with a comment as to WAKE_ALARM being deprecated), so its more clear
> >> which is the one that ought to be used by new code.
> >>
> >> However, in the spirit of this thread, we might even consider
> >> broadening the cap silo a bit further, to something like
> >> CAP_WAKE_SUSPEND, such that it might also be able to cover broader PM
> >> actions?
> >
> > Or just CAP_UNSUSPEND?
> 
> I guess I was trying to capture it could be use for actions like both
> waking and suspending the system.

Well, CAP_SYS_PM comes to mind then.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index fd4f87d..ba972ff 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -357,6 +357,8 @@  struct vfs_ns_cap_data {
 
 #define CAP_WAKE_ALARM            35
 
+#define CAP_WAKE_SYSTEM      CAP_WAKE_ALARM
+
 /* Allow preventing system suspends */
 
 #define CAP_BLOCK_SUSPEND    36