[PATCH] lsm,nfs: fix memory leak of lsm_context

[Casey Schaufler]

On 2/21/2025 12:10 PM, Paul Moore wrote:
> On Thu, Feb 20, 2025 at 2:30 PM Stephen Smalley
> <stephen.smalley.work at gmail.com> wrote:
>> 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 at gmail.com>
>> ---
>>  fs/nfs/nfs4proc.c    | 7 ++++---
>>  include/linux/nfs4.h | 1 +
>>  2 files changed, 5 insertions(+), 3 deletions(-)
> Now that we've seen Casey's patch, I believe this patch is the better
> solution and we should get this up to Linus during the v6.14-rcX
> phase.  I've got one minor nitpick (below), but how would you like to
> handle this patch NFS folks?  I'm guessing you would prefer to pull
> this via the NFS tree, but if not let me know and I can pull it via
> the LSM tree (an ACK would be appreciated).
>
> Acked-by: Paul Moore <paul at paul-moore.com>

If you like that approach better it should use a lsm_context struct for
the nfs data rather than adding a lsm_id. Would you entertain that change?

>
>> 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;
> I don't think this is a significant problem, but considering that
> lsm_context::id is an int, the lsmid field above should probably be an
> int too.
>
>>         u32             len;
>>         char    *label;
>>  };
>> --
>> 2.47.1