| Message ID | 20240830003411.16818-2-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | LSM: Move away from secids | expand |
On Aug 29, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > > When more than one security module is exporting data to audit and > networking sub-systems a single 32 bit integer is no longer > sufficient to represent the data. Add a structure to be used instead. > > The lsmblob structure definition is intended to keep the LSM > specific information private to the individual security modules. > The module specific information is included in a new set of > header files under include/lsm. Each security module is allowed > to define the information included for its use in the lsmblob. > SELinux includes a u32 secid. Smack includes a pointer into its > global label list. The conditional compilation based on feature > inclusion is contained in the include/lsm files. > > Suggested-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > Cc: apparmor@lists.ubuntu.com > Cc: bpf@vger.kernel.org > Cc: selinux@vger.kernel.org > Cc: linux-security-module@vger.kernel.org > --- > include/linux/lsm/apparmor.h | 17 +++++++++++++++++ > include/linux/lsm/bpf.h | 16 ++++++++++++++++ > include/linux/lsm/selinux.h | 16 ++++++++++++++++ > include/linux/lsm/smack.h | 17 +++++++++++++++++ > include/linux/security.h | 20 ++++++++++++++++++++ > 5 files changed, 86 insertions(+) > create mode 100644 include/linux/lsm/apparmor.h > create mode 100644 include/linux/lsm/bpf.h > create mode 100644 include/linux/lsm/selinux.h > create mode 100644 include/linux/lsm/smack.h ... > diff --git a/include/linux/security.h b/include/linux/security.h > index 1390f1efb4f0..0057a22137e8 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -34,6 +34,10 @@ > #include <linux/sockptr.h> > #include <linux/bpf.h> > #include <uapi/linux/lsm.h> > +#include <linux/lsm/selinux.h> > +#include <linux/lsm/smack.h> > +#include <linux/lsm/apparmor.h> > +#include <linux/lsm/bpf.h> > > struct linux_binprm; > struct cred; > @@ -140,6 +144,22 @@ enum lockdown_reason { > LOCKDOWN_CONFIDENTIALITY_MAX, > }; > > +/* scaffolding */ > +struct lsmblob_scaffold { > + u32 secid; > +}; > + > +/* > + * Data exported by the security modules > + */ > +struct lsmblob { > + struct lsmblob_selinux selinux; > + struct lsmblob_smack smack; > + struct lsmblob_apparmor apparmor; > + struct lsmblob_bpf bpf; > + struct lsmblob_scaffold scaffold; > +}; Warning, top shelf bikeshedding follows ... I believe that historically when we've talked about the "LSM blob" we've usually been referring to the opaque buffers used to store LSM state that we attach to a number of kernel structs using the `void *security` field. At least that is what I think of when I read "struct lsmblob", and I'd like to get ahead of the potential confusion while we still can. Casey, I'm sure you're priority is simply getting this merged and you likely care very little about the name (as long as it isn't too horrible), but what about "lsm_ref"? Other ideas are most definitely welcome. I'm not going to comment on all the other related occurrences in the patchset, but all the "XXX_lsmblob_XXX" functions should be adjusted based on what we name the struct, e.g. "XXX_lsmref_XXX". -- paul-moore.com
On 9/3/2024 5:18 PM, Paul Moore wrote: > On Aug 29, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: >> When more than one security module is exporting data to audit and >> networking sub-systems a single 32 bit integer is no longer >> sufficient to represent the data. Add a structure to be used instead. >> >> The lsmblob structure definition is intended to keep the LSM >> specific information private to the individual security modules. >> The module specific information is included in a new set of >> header files under include/lsm. Each security module is allowed >> to define the information included for its use in the lsmblob. >> SELinux includes a u32 secid. Smack includes a pointer into its >> global label list. The conditional compilation based on feature >> inclusion is contained in the include/lsm files. >> >> Suggested-by: Paul Moore <paul@paul-moore.com> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> Cc: apparmor@lists.ubuntu.com >> Cc: bpf@vger.kernel.org >> Cc: selinux@vger.kernel.org >> Cc: linux-security-module@vger.kernel.org >> --- >> include/linux/lsm/apparmor.h | 17 +++++++++++++++++ >> include/linux/lsm/bpf.h | 16 ++++++++++++++++ >> include/linux/lsm/selinux.h | 16 ++++++++++++++++ >> include/linux/lsm/smack.h | 17 +++++++++++++++++ >> include/linux/security.h | 20 ++++++++++++++++++++ >> 5 files changed, 86 insertions(+) >> create mode 100644 include/linux/lsm/apparmor.h >> create mode 100644 include/linux/lsm/bpf.h >> create mode 100644 include/linux/lsm/selinux.h >> create mode 100644 include/linux/lsm/smack.h > .. > >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 1390f1efb4f0..0057a22137e8 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -34,6 +34,10 @@ >> #include <linux/sockptr.h> >> #include <linux/bpf.h> >> #include <uapi/linux/lsm.h> >> +#include <linux/lsm/selinux.h> >> +#include <linux/lsm/smack.h> >> +#include <linux/lsm/apparmor.h> >> +#include <linux/lsm/bpf.h> >> >> struct linux_binprm; >> struct cred; >> @@ -140,6 +144,22 @@ enum lockdown_reason { >> LOCKDOWN_CONFIDENTIALITY_MAX, >> }; >> >> +/* scaffolding */ >> +struct lsmblob_scaffold { >> + u32 secid; >> +}; >> + >> +/* >> + * Data exported by the security modules >> + */ >> +struct lsmblob { >> + struct lsmblob_selinux selinux; >> + struct lsmblob_smack smack; >> + struct lsmblob_apparmor apparmor; >> + struct lsmblob_bpf bpf; >> + struct lsmblob_scaffold scaffold; >> +}; > Warning, top shelf bikeshedding follows ... Not unexpected. :) > I believe that historically when we've talked about the "LSM blob" we've > usually been referring to the opaque buffers used to store LSM state that > we attach to a number of kernel structs using the `void *security` field. > > At least that is what I think of when I read "struct lsmblob", and I'd > like to get ahead of the potential confusion while we still can. > > Casey, I'm sure you're priority is simply getting this merged and you > likely care very little about the name (as long as it isn't too horrible), I would reject lsmlatefordinner out of hand. > but what about "lsm_ref"? Other ideas are most definitely welcome. I'm not a fan of the underscore, and ref seems to imply memory management. How about "struct lsmsecid", which is a nod to the past "u32 secid"? Or, "struct lsmdata", "struct lsmid", "struct lsmattr". I could live with "struct lsmref", I suppose, although it pulls me toward "struct lsmreference", which is a bit long. > I'm not going to comment on all the other related occurrences in the > patchset, but all the "XXX_lsmblob_XXX" functions should be adjusted based > on what we name the struct, e.g. "XXX_lsmref_XXX". > > -- > paul-moore.com >
On Tue, Sep 3, 2024 at 8:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 9/3/2024 5:18 PM, Paul Moore wrote: > > On Aug 29, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: ... > >> +/* > >> + * Data exported by the security modules > >> + */ > >> +struct lsmblob { > >> + struct lsmblob_selinux selinux; > >> + struct lsmblob_smack smack; > >> + struct lsmblob_apparmor apparmor; > >> + struct lsmblob_bpf bpf; > >> + struct lsmblob_scaffold scaffold; > >> +}; > > > > Warning, top shelf bikeshedding follows ... > > Not unexpected. :) > > > I believe that historically when we've talked about the "LSM blob" we've > > usually been referring to the opaque buffers used to store LSM state that > > we attach to a number of kernel structs using the `void *security` field. > > > > At least that is what I think of when I read "struct lsmblob", and I'd > > like to get ahead of the potential confusion while we still can. > > > > Casey, I'm sure you're priority is simply getting this merged and you > > likely care very little about the name (as long as it isn't too horrible), > > I would reject lsmlatefordinner out of hand. Fair enough :) > > but what about "lsm_ref"? Other ideas are most definitely welcome. > > I'm not a fan of the underscore, and ref seems to imply memory management. > How about "struct lsmsecid", which is a nod to the past "u32 secid"? > Or, "struct lsmdata", "struct lsmid", "struct lsmattr". > I could live with "struct lsmref", I suppose, although it pulls me toward > "struct lsmreference", which is a bit long. For what it's worth, I do agree that "ref" is annoyingly similar to a reference counter, I don't love it here, but I'm having a hard time coming up with something appropriate. I also tend to like the underscore, at least in the struct name, as it matches well with the "lsm_ctx" struct we have as part of the UAPI. When we use the struct name in function names, feel free to drop the underscore, for example: "lsm_foo" -> "security_get_lsmfoo()". My first thought was for something like "lsmid" (ignoring the underscore debate), but we already have the LSM_ID_XXX defines which are something entirely different and I felt like we would be trading one source of confusion for another. There is a similar problem with the LSM_ATTR_XXX defines. We also already have a "lsm_ctx" struct which sort of rules out "lsmctx" for what are hopefully obvious reasons. I'd also like to avoid anything involving "secid" or "secctx" simply because the whole point of this struct is to move past the idea of a single integer or string representing all of the LSM properties for an entity. I can understand "lsm_data", but that is more ambiguous than I would like. What about "lsm_prop" or "lsm_cred"?
On 9/4/2024 1:00 PM, Paul Moore wrote: > On Tue, Sep 3, 2024 at 8:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 9/3/2024 5:18 PM, Paul Moore wrote: >>> On Aug 29, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > .. > >>>> +/* >>>> + * Data exported by the security modules >>>> + */ >>>> +struct lsmblob { >>>> + struct lsmblob_selinux selinux; >>>> + struct lsmblob_smack smack; >>>> + struct lsmblob_apparmor apparmor; >>>> + struct lsmblob_bpf bpf; >>>> + struct lsmblob_scaffold scaffold; >>>> +}; >>> Warning, top shelf bikeshedding follows ... >> Not unexpected. :) >> >>> I believe that historically when we've talked about the "LSM blob" we've >>> usually been referring to the opaque buffers used to store LSM state that >>> we attach to a number of kernel structs using the `void *security` field. >>> >>> At least that is what I think of when I read "struct lsmblob", and I'd >>> like to get ahead of the potential confusion while we still can. >>> >>> Casey, I'm sure you're priority is simply getting this merged and you >>> likely care very little about the name (as long as it isn't too horrible), >> I would reject lsmlatefordinner out of hand. > Fair enough :) > >>> but what about "lsm_ref"? Other ideas are most definitely welcome. >> I'm not a fan of the underscore, and ref seems to imply memory management. >> How about "struct lsmsecid", which is a nod to the past "u32 secid"? >> Or, "struct lsmdata", "struct lsmid", "struct lsmattr". >> I could live with "struct lsmref", I suppose, although it pulls me toward >> "struct lsmreference", which is a bit long. > For what it's worth, I do agree that "ref" is annoyingly similar to a > reference counter, I don't love it here, but I'm having a hard time > coming up with something appropriate. > > I also tend to like the underscore, at least in the struct name, as it > matches well with the "lsm_ctx" struct we have as part of the UAPI. > When we use the struct name in function names, feel free to drop the > underscore, for example: "lsm_foo" -> "security_get_lsmfoo()". > > My first thought was for something like "lsmid" (ignoring the > underscore debate), but we already have the LSM_ID_XXX defines which > are something entirely different and I felt like we would be trading > one source of confusion for another. There is a similar problem with > the LSM_ATTR_XXX defines. > > We also already have a "lsm_ctx" struct which sort of rules out > "lsmctx" for what are hopefully obvious reasons. > > I'd also like to avoid anything involving "secid" or "secctx" simply > because the whole point of this struct is to move past the idea of a > single integer or string representing all of the LSM properties for an > entity. > > I can understand "lsm_data", but that is more ambiguous than I would like. > > What about "lsm_prop" or "lsm_cred"? If we ever do the same sort of thing for the existing blobs we're going to need to have lsm_cred for the cred blob, so I shan't use it here. I can live with lsm_prop, which shouldn't confuse too many developers. We can start saying "property" in place of secid, which would be a good thing.
On Wed, Sep 4, 2024 at 4:28 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 9/4/2024 1:00 PM, Paul Moore wrote: > > On Tue, Sep 3, 2024 at 8:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 9/3/2024 5:18 PM, Paul Moore wrote: > >>> On Aug 29, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > > .. > > > >>>> +/* > >>>> + * Data exported by the security modules > >>>> + */ > >>>> +struct lsmblob { > >>>> + struct lsmblob_selinux selinux; > >>>> + struct lsmblob_smack smack; > >>>> + struct lsmblob_apparmor apparmor; > >>>> + struct lsmblob_bpf bpf; > >>>> + struct lsmblob_scaffold scaffold; > >>>> +}; > >>> Warning, top shelf bikeshedding follows ... > >> Not unexpected. :) > >> > >>> I believe that historically when we've talked about the "LSM blob" we've > >>> usually been referring to the opaque buffers used to store LSM state that > >>> we attach to a number of kernel structs using the `void *security` field. > >>> > >>> At least that is what I think of when I read "struct lsmblob", and I'd > >>> like to get ahead of the potential confusion while we still can. > >>> > >>> Casey, I'm sure you're priority is simply getting this merged and you > >>> likely care very little about the name (as long as it isn't too horrible), > >> I would reject lsmlatefordinner out of hand. > > Fair enough :) > > > >>> but what about "lsm_ref"? Other ideas are most definitely welcome. > >> I'm not a fan of the underscore, and ref seems to imply memory management. > >> How about "struct lsmsecid", which is a nod to the past "u32 secid"? > >> Or, "struct lsmdata", "struct lsmid", "struct lsmattr". > >> I could live with "struct lsmref", I suppose, although it pulls me toward > >> "struct lsmreference", which is a bit long. > > For what it's worth, I do agree that "ref" is annoyingly similar to a > > reference counter, I don't love it here, but I'm having a hard time > > coming up with something appropriate. > > > > I also tend to like the underscore, at least in the struct name, as it > > matches well with the "lsm_ctx" struct we have as part of the UAPI. > > When we use the struct name in function names, feel free to drop the > > underscore, for example: "lsm_foo" -> "security_get_lsmfoo()". > > > > My first thought was for something like "lsmid" (ignoring the > > underscore debate), but we already have the LSM_ID_XXX defines which > > are something entirely different and I felt like we would be trading > > one source of confusion for another. There is a similar problem with > > the LSM_ATTR_XXX defines. > > > > We also already have a "lsm_ctx" struct which sort of rules out > > "lsmctx" for what are hopefully obvious reasons. > > > > I'd also like to avoid anything involving "secid" or "secctx" simply > > because the whole point of this struct is to move past the idea of a > > single integer or string representing all of the LSM properties for an > > entity. > > > > I can understand "lsm_data", but that is more ambiguous than I would like. > > > > What about "lsm_prop" or "lsm_cred"? > > If we ever do the same sort of thing for the existing blobs we're > going to need to have lsm_cred for the cred blob, so I shan't use > it here. I can live with lsm_prop, which shouldn't confuse too many > developers. We can start saying "property" in place of secid, which > would be a good thing. Works for me, thanks Casey.
diff --git a/include/linux/lsm/apparmor.h b/include/linux/lsm/apparmor.h new file mode 100644 index 000000000000..11521f66d548 --- /dev/null +++ b/include/linux/lsm/apparmor.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Linux Security Module interface to other subsystems. + * AppArmor presents single pointer to an aa_label structure. + */ +#ifndef __LINUX_LSM_APPARMOR_H +#define __LINUX_LSM_APPARMOR_H + +struct aa_label; + +struct lsmblob_apparmor { +#ifdef CONFIG_SECURITY_APPARMOR + struct aa_label *label; +#endif +}; + +#endif /* ! __LINUX_LSM_APPARMOR_H */ diff --git a/include/linux/lsm/bpf.h b/include/linux/lsm/bpf.h new file mode 100644 index 000000000000..48abdcd82ded --- /dev/null +++ b/include/linux/lsm/bpf.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Linux Security Module interface to other subsystems. + * BPF may present a single u32 value. + */ +#ifndef __LINUX_LSM_BPF_H +#define __LINUX_LSM_BPF_H +#include <linux/types.h> + +struct lsmblob_bpf { +#ifdef CONFIG_BPF_LSM + u32 secid; +#endif +}; + +#endif /* ! __LINUX_LSM_BPF_H */ diff --git a/include/linux/lsm/selinux.h b/include/linux/lsm/selinux.h new file mode 100644 index 000000000000..fd16456b36ac --- /dev/null +++ b/include/linux/lsm/selinux.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Linux Security Module interface to other subsystems. + * SELinux presents a single u32 value which is known as a secid. + */ +#ifndef __LINUX_LSM_SELINUX_H +#define __LINUX_LSM_SELINUX_H +#include <linux/types.h> + +struct lsmblob_selinux { +#ifdef CONFIG_SECURITY_SELINUX + u32 secid; +#endif +}; + +#endif /* ! __LINUX_LSM_SELINUX_H */ diff --git a/include/linux/lsm/smack.h b/include/linux/lsm/smack.h new file mode 100644 index 000000000000..2018f288302f --- /dev/null +++ b/include/linux/lsm/smack.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Linux Security Module interface to other subsystems. + * Smack presents a pointer into the global Smack label list. + */ +#ifndef __LINUX_LSM_SMACK_H +#define __LINUX_LSM_SMACK_H + +struct smack_known; + +struct lsmblob_smack { +#ifdef CONFIG_SECURITY_SMACK + struct smack_known *skp; +#endif +}; + +#endif /* ! __LINUX_LSM_SMACK_H */ diff --git a/include/linux/security.h b/include/linux/security.h index 1390f1efb4f0..0057a22137e8 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -34,6 +34,10 @@ #include <linux/sockptr.h> #include <linux/bpf.h> #include <uapi/linux/lsm.h> +#include <linux/lsm/selinux.h> +#include <linux/lsm/smack.h> +#include <linux/lsm/apparmor.h> +#include <linux/lsm/bpf.h> struct linux_binprm; struct cred; @@ -140,6 +144,22 @@ enum lockdown_reason { LOCKDOWN_CONFIDENTIALITY_MAX, }; +/* scaffolding */ +struct lsmblob_scaffold { + u32 secid; +}; + +/* + * Data exported by the security modules + */ +struct lsmblob { + struct lsmblob_selinux selinux; + struct lsmblob_smack smack; + struct lsmblob_apparmor apparmor; + struct lsmblob_bpf bpf; + struct lsmblob_scaffold scaffold; +}; + extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1]; extern u32 lsm_active_cnt; extern const struct lsm_id *lsm_idlist[];
When more than one security module is exporting data to audit and networking sub-systems a single 32 bit integer is no longer sufficient to represent the data. Add a structure to be used instead. The lsmblob structure definition is intended to keep the LSM specific information private to the individual security modules. The module specific information is included in a new set of header files under include/lsm. Each security module is allowed to define the information included for its use in the lsmblob. SELinux includes a u32 secid. Smack includes a pointer into its global label list. The conditional compilation based on feature inclusion is contained in the include/lsm files. Suggested-by: Paul Moore <paul@paul-moore.com> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> Cc: apparmor@lists.ubuntu.com Cc: bpf@vger.kernel.org Cc: selinux@vger.kernel.org Cc: linux-security-module@vger.kernel.org --- include/linux/lsm/apparmor.h | 17 +++++++++++++++++ include/linux/lsm/bpf.h | 16 ++++++++++++++++ include/linux/lsm/selinux.h | 16 ++++++++++++++++ include/linux/lsm/smack.h | 17 +++++++++++++++++ include/linux/security.h | 20 ++++++++++++++++++++ 5 files changed, 86 insertions(+) create mode 100644 include/linux/lsm/apparmor.h create mode 100644 include/linux/lsm/bpf.h create mode 100644 include/linux/lsm/selinux.h create mode 100644 include/linux/lsm/smack.h