[PATCH 2/6] LSM: Infrastructure management of the key security blob

[Paul Moore]

On Tue, Jul 9, 2024 at 6:47 PM John Johansen
<john.johansen at canonical.com> wrote:
> On 7/9/24 15:08, Paul Moore wrote:
> > On Jul  8, 2024 Casey Schaufler <casey at schaufler-ca.com> wrote:
> >>
> >> Move management of the key->security blob out of the
> >> individual security modules and into the security
> >> infrastructure. Instead of allocating the blobs from within
> >> the modules the modules tell the infrastructure how much
> >> space is required, and the space is allocated there.
> >
> > Perhaps mention that the key_free hook is being removed as it is not
> > currently needed after this change?
> >
> >> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
> >> ---
> >>   include/linux/lsm_hooks.h         |  1 +
> >>   security/security.c               | 41 +++++++++++++++++++++++++++++--
> >>   security/selinux/hooks.c          | 23 +++++------------
> >>   security/selinux/include/objsec.h |  7 ++++++
> >>   security/smack/smack.h            |  7 ++++++
> >>   security/smack/smack_lsm.c        | 33 +++++++++++--------------
> >>   6 files changed, 75 insertions(+), 37 deletions(-)

...

> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 19346e1817ff..b3de2e941ef7 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -6981,6 +6968,9 @@ struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
> >>      .lbs_file = sizeof(struct file_security_struct),
> >>      .lbs_inode = sizeof(struct inode_security_struct),
> >>      .lbs_ipc = sizeof(struct ipc_security_struct),
> >> +#ifdef CONFIG_KEYS
> >> +    .lbs_key = sizeof(struct key_security_struct),
> >> +#endif /* CONFIG_KEYS */
> >
> > We can probably get rid of the Kconfig conditional.  I understand the
> > desire to keep this to only what is needed, but since this only really
> > has an impact on boot, and the impact is some basic math, I'd rather
> > not run the risk of rot due to conditional compilation.
>
> sure, the counter argument is why isn't this conditional when other parts
> for keys is. That inconsistency introduces it own issues that can cause
> problems down the road. I really don't have an opinion on which way
> is better, just playing devils advocate.

That's fair, since it's Casey's patch I guess he can be the tie break.
While I generally prefer to limit conditional code blocks, I'm not
overly concerned about this one.

> >>      .lbs_msg_msg = sizeof(struct msg_security_struct),
> >>      .lbs_sock = sizeof(struct sk_security_struct),
> >>      .lbs_superblock = sizeof(struct superblock_security_struct),
> >
> > ...
> >
> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >> index a931b44bc959..17bcc9cbf584 100644
> >> --- a/security/smack/smack_lsm.c
> >> +++ b/security/smack/smack_lsm.c
> >> @@ -5010,6 +5005,9 @@ struct lsm_blob_sizes smack_blob_sizes __ro_after_init = {
> >>      .lbs_file = sizeof(struct smack_known *),
> >>      .lbs_inode = sizeof(struct inode_smack),
> >>      .lbs_ipc = sizeof(struct smack_known *),
> >> +#ifdef CONFIG_KEYS
> >> +    .lbs_key = sizeof(struct smack_known *),
> >> +#endif /* CONFIG_KEYS */
> >
> > See above, but ultimately this is Smack code so it's your call.
>
> I think we should be consistent, either it goes everywhere or it stays
> everywhere. It is going to be confusing for other people coming in and
> looking at the code as to why one is conditional and one isn't.

I agree with everything you've said, but I personally don't want to be
in the business of demanding things from the individual LSMs so long
as they aren't hurting anything else.  Hopefully Casey will go for
consistency ;)

-- 
paul-moore.com