Re: [PATCH v3 4/5] LSM: lsm_context in security_dentry_init_security

[Casey Schaufler]

On 2/20/2025 11:37 AM, Stephen Smalley wrote:
> On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> On 2/20/2025 10:16 AM, Stephen Smalley wrote:
>>> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley
>>> <stephen.smalley.work@xxxxxxxxx> wrote:
>>>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>>>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>>>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley
>>>>>> <stephen.smalley.work@xxxxxxxxx> wrote:
>>>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>>>>>>>> Replace the (secctx,seclen) pointer pair with a single lsm_context
>>>>>>>> pointer to allow return of the LSM identifier along with the context
>>>>>>>> and context length. This allows security_release_secctx() to know how
>>>>>>>> to release the context. Callers have been modified to use or save the
>>>>>>>> returned data from the new structure.
>>>>>>>>
>>>>>>>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>>>>>>> Cc: ceph-devel@xxxxxxxxxxxxxxx
>>>>>>>> Cc: linux-nfs@xxxxxxxxxxxxxxx
>>>>>>>> ---
>>>>>>>>  fs/ceph/super.h               |  3 +--
>>>>>>>>  fs/ceph/xattr.c               | 16 ++++++----------
>>>>>>>>  fs/fuse/dir.c                 | 35 ++++++++++++++++++-----------------
>>>>>>>>  fs/nfs/nfs4proc.c             | 20 ++++++++++++--------
>>>>>>>>  include/linux/lsm_hook_defs.h |  2 +-
>>>>>>>>  include/linux/security.h      | 26 +++-----------------------
>>>>>>>>  security/security.c           |  9 ++++-----
>>>>>>>>  security/selinux/hooks.c      |  9 +++++----
>>>>>>>>  8 files changed, 50 insertions(+), 70 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>>>>> index 76776d716744..0b116ef3a752 100644
>>>>>>>> --- a/fs/nfs/nfs4proc.c
>>>>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>>>>> @@ -114,6 +114,7 @@ static inline struct nfs4_label *
>>>>>>>>  nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
>>>>>>>>         struct iattr *sattr, struct nfs4_label *label)
>>>>>>>>  {
>>>>>>>> +       struct lsm_context shim;
>>>>>>>>         int err;
>>>>>>>>
>>>>>>>>         if (label == NULL)
>>>>>>>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
>>>>>>>>         label->label = NULL;
>>>>>>>>
>>>>>>>>         err = security_dentry_init_security(dentry, sattr->ia_mode,
>>>>>>>> -                               &dentry->d_name, NULL,
>>>>>>>> -                               (void **)&label->label, &label->len);
>>>>>>>> -       if (err == 0)
>>>>>>>> -               return label;
>>>>>>>> +                               &dentry->d_name, NULL, &shim);
>>>>>>>> +       if (err)
>>>>>>>> +               return NULL;
>>>>>>>>
>>>>>>>> -       return NULL;
>>>>>>>> +       label->label = shim.context;
>>>>>>>> +       label->len = shim.len;
>>>>>>>> +       return label;
>>>>>>>>  }
>>>>>>>>  static inline void
>>>>>>>>  nfs4_label_release_security(struct nfs4_label *label)
>>>>>>>>  {
>>>>>>>> -       struct lsm_context scaff; /* scaffolding */
>>>>>>>> +       struct lsm_context shim;
>>>>>>>>
>>>>>>>>         if (label) {
>>>>>>>> -               lsmcontext_init(&scaff, label->label, label->len, 0);
>>>>>>>> -               security_release_secctx(&scaff);
>>>>>>>> +               shim.context = label->label;
>>>>>>>> +               shim.len = label->len;
>>>>>>>> +               shim.id = LSM_ID_UNDEF;
>>>>>>> Is there a patch that follows this one to fix this? Otherwise, setting
>>>>>>> this to UNDEF causes SELinux to NOT free the context, which produces a
>>>>>>> memory leak for every NFS inode security context. Reported by kmemleak
>>>>>>> when running the selinux-testsuite NFS tests.
>>>>>> I don't recall seeing anything related to this, but patches are
>>>>>> definitely welcome.
>>>>> Looking at this quickly, this is an interesting problem as I don't
>>>>> believe we have enough context in nfs4_label_release_security() to
>>>>> correctly set the shim.id value.  If there is a positive, it is that
>>>>> lsm_context is really still just a string wrapped up with some
>>>>> metadata, e.g. length/ID, so we kfree()'ing shim.context is going to
>>>>> be okay-ish, at least for the foreseeable future.
>>>>>
>>>>> I can think of two ways to fix this, but I'd love to hear other ideas too.
>>>>>
>>>>> 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx()
>>>>> and skip any individual LSM processing.
>>>>>
>>>>> 2. Define a new LSM_ID_ANY value and update all of the LSMs to also
>>>>> process the ANY case as well as their own.
>>>>>
>>>>> I'm not finding either option very exciting, but option #2 looks
>>>>> particularly ugly, so I think I'd prefer to see someone draft a patch
>>>>> for option #1 assuming nothing better is presented.
>>>> We could perhaps add a u32 lsmid to struct nfs4_label, save it from
>>>> the shim.id obtained in nfs4_label_init_security(), and use it in
>>>> nfs4_label_release_security(). Not sure why that wasn't done in the
>>>> first place.
>>> Something like this (not tested yet). If this looks sane, will submit
>>> separately.
>>>
>>> commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security")
>>> did not preserve the lsm id for subsequent release calls, which results
>>> in a memory leak. Fix it by saving the lsm id in the nfs4_label and
>>> providing it on the subsequent release call.
>>>
>>> Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security")
>>> Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
>> I'm not a fan of adding secids into other subsystems, especially in cases
>> where they've tried to avoid them in the past.
>>
>> The better solution, which I'm tracking down the patch for now, is for
>> the individual LSMs to always do their release, and for security_release_secctx()
>> to check the lsm_id and call the appropriate LSM specific hook. Until there
>> are multiple LSMs with contexts, LSM_ID_UNDEF is as good as a match.
>>
>> Please don't use this patch.
> It doesn't add a secid; it just saves the LSM id obtained from
> lsm_context populated by the security_dentry_init_security() hook call
> and passes it back in the lsm_context to the security_release_secctx()
> call.

Right. Sorry. If you're going to do that, the nfs_label struct should
just include a lsm_context instead. But that hit opposition when proposed
initially.

The practical solution has to acknowledge that at this stage there can only
be one LSM providing contexts, and each LSM can release the context if the
LSM is matches the LSM or is LSM_ID_UNDEF. That will change before SELinux,
AppArmor and Smack can co-exist, but that's not yet available. For now the
check

	if (cp->id == LSM_ID_SELINUX)

can either be removed or changed to

	if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF)

In a system that respects LSM_FLAG_LEGACY_MAJOR the id isn't relevant
with the context using LSMs all being thus identified.

>
>>> ---
>>>  fs/nfs/nfs4proc.c    | 7 ++++---
>>>  include/linux/nfs4.h | 1 +
>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index df9669d4ded7..c0caaec7bd20 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -133,6 +133,7 @@ nfs4_label_init_security(struct inode *dir, struct
>>> dentry *dentry,
>>>   if (err)
>>>   return NULL;
>>>
>>> + label->lsmid = shim.id;
>>>   label->label = shim.context;
>>>   label->len = shim.len;
>>>   return label;
>>> @@ -145,7 +146,7 @@ nfs4_label_release_security(struct nfs4_label *label)
>>>   if (label) {
>>>   shim.context = label->label;
>>>   shim.len = label->len;
>>> - shim.id = LSM_ID_UNDEF;
>>> + shim.id = label->lsmid;
>>>   security_release_secctx(&shim);
>>>   }
>>>  }
>>> @@ -6269,7 +6270,7 @@ static int _nfs4_get_security_label(struct inode
>>> *inode, void *buf,
>>>   size_t buflen)
>>>  {
>>>   struct nfs_server *server = NFS_SERVER(inode);
>>> - struct nfs4_label label = {0, 0, buflen, buf};
>>> + struct nfs4_label label = {0, 0, 0, buflen, buf};
>>>
>>>   u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL };
>>>   struct nfs_fattr fattr = {
>>> @@ -6374,7 +6375,7 @@ static int nfs4_do_set_security_label(struct inode *inode,
>>>  static int
>>>  nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen)
>>>  {
>>> - struct nfs4_label ilabel = {0, 0, buflen, (char *)buf };
>>> + struct nfs4_label ilabel = {0, 0, 0, buflen, (char *)buf };
>>>   struct nfs_fattr *fattr;
>>>   int status;
>>>
>>> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
>>> index 71fbebfa43c7..9ac83ca88326 100644
>>> --- a/include/linux/nfs4.h
>>> +++ b/include/linux/nfs4.h
>>> @@ -47,6 +47,7 @@ struct nfs4_acl {
>>>  struct nfs4_label {
>>>   uint32_t lfs;
>>>   uint32_t pi;
>>> + u32 lsmid;
>>>   u32 len;
>>>   char *label;
>>>  };