| Message ID | 20250220192935.9014-2-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | lsm,nfs: fix memory leak of lsm_context | expand |
On 2/20/2025 11:29 AM, Stephen Smalley 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@gmail.com> Please don't accept this. I will have a better, simpler, more appropriate fix promptly. > --- > 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; > };
On Thu, Feb 20, 2025 at 2:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 2/20/2025 11:29 AM, Stephen Smalley 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@gmail.com> > > Please don't accept this. I will have a better, simpler, more appropriate > fix promptly. We're still only at -rc3 so if you need another couple of days to get an alternate solution out for review/discussion I think that's okay. However, the solution proposed here is rather small and easy to understand, which has a lot of value for an -rcX fix. If the alternative is notably larger and/or more complex, the patch here may be preferable for v6.14 with the alternative/"proper-fix" being something we can queue up for the next merge window. Regardless, let's see Casey's fix before we worry too much about this. > > --- > > 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; > > };
On Thu, Feb 20, 2025 at 2:30 PM Stephen Smalley <stephen.smalley.work@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@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@paul-moore.com> > 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
On 2/21/2025 12:10 PM, Paul Moore wrote: > On Thu, Feb 20, 2025 at 2:30 PM Stephen Smalley > <stephen.smalley.work@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@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@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
On Fri, Feb 21, 2025 at 3:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 2/21/2025 12:10 PM, Paul Moore wrote: > > On Thu, Feb 20, 2025 at 2:30 PM Stephen Smalley > > <stephen.smalley.work@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@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@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? I had considered that approach but doing so would require changing every user of nfs4_label to use the lsm_context fields instead of the current label/len fields (unless you are going to duplicate/alias them). And not all of these originate from an lsm_context IIUC.
On 2/21/2025 12:42 PM, Stephen Smalley wrote: > On Fri, Feb 21, 2025 at 3:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 2/21/2025 12:10 PM, Paul Moore wrote: >>> On Thu, Feb 20, 2025 at 2:30 PM Stephen Smalley >>> <stephen.smalley.work@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@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@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? > I had considered that approach but doing so would require changing > every user of nfs4_label to use the lsm_context fields instead of the > current label/len fields (unless you are going to duplicate/alias > them). And not all of these originate from an lsm_context IIUC. Sigh. I would like to disagree with you, but I can't. You can add my Acked-by: Casey Schaufler <casey@schaufler-ca.com> >
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; };
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@gmail.com> --- fs/nfs/nfs4proc.c | 7 ++++--- include/linux/nfs4.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)