Message ID | 1587361410-83560-1-git-send-email-xiyuyang19@fudan.edu.cn (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl | expand |
On Mon, 2020-04-20 at 13:43 +0800, Xiyu Yang wrote: > nfs3_set_acl() invokes get_acl(), which returns a local reference of > the > posix_acl object to "alloc" with increased refcount. > > When nfs3_set_acl() returns or a new object is assigned to > "alloc", the > original local reference of "alloc" becomes invalid, so the refcount > should be decreased to keep refcount balanced. > > The reference counting issue happens in one path of nfs3_set_acl(). > When > "acl" equals to NULL but "alloc" is not NULL, the function forgets to > decrease the refcnt increased by get_acl() and causes a refcnt leak. > > Fix this issue by calling posix_acl_release() on this path when > "alloc" > is not NULL. > > Fixes: b7fa0554cf1b ("[PATCH] NFS: Add support for NFSv3 ACLs") > Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> > Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> > --- > fs/nfs/nfs3acl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c > index c5c3fc6e6c60..b5c41bcca8cf 100644 > --- a/fs/nfs/nfs3acl.c > +++ b/fs/nfs/nfs3acl.c > @@ -274,6 +274,8 @@ int nfs3_set_acl(struct inode *inode, struct > posix_acl *acl, int type) > } > > if (acl == NULL) { > + if (alloc) > + posix_acl_release(alloc); This will result in 'dfacl' being freed while it is still in use, so can't be correct either. > alloc = acl = posix_acl_from_mode(inode->i_mode, > GFP_KERNEL); > if (IS_ERR(alloc)) > goto fail; I don't really see any alternative to adding a 'dfalloc' to track the allocated dfacl separately.
Am Mo., 20. Apr. 2020 um 14:15 Uhr schrieb Trond Myklebust <trondmy@hammerspace.com>: > I don't really see any alternative to adding a 'dfalloc' to track the > allocated dfacl separately. Something like the attached patch should work as well. Thanks, Andreas --- fs/nfs/nfs3acl.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c index c5c3fc6e6c60..f1581f11c220 100644 --- a/fs/nfs/nfs3acl.c +++ b/fs/nfs/nfs3acl.c @@ -253,37 +253,41 @@ int nfs3_proc_setacls(struct inode *inode, struct posix_acl *acl, int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int type) { - struct posix_acl *alloc = NULL, *dfacl = NULL; + struct posix_acl *orig = acl, *dfacl = NULL; int status; if (S_ISDIR(inode->i_mode)) { switch(type) { case ACL_TYPE_ACCESS: - alloc = dfacl = get_acl(inode, ACL_TYPE_DEFAULT); - if (IS_ERR(alloc)) - goto fail; + dfacl = get_acl(inode, ACL_TYPE_DEFAULT); + status = PTR_ERR(dfacl); + if (IS_ERR(dfacl)) + goto out; break; case ACL_TYPE_DEFAULT: dfacl = acl; - alloc = acl = get_acl(inode, ACL_TYPE_ACCESS); - if (IS_ERR(alloc)) - goto fail; + acl = get_acl(inode, ACL_TYPE_ACCESS); + status = PTR_ERR(acl); + if (IS_ERR(acl)) + goto out; break; } } if (acl == NULL) { - alloc = acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL); - if (IS_ERR(alloc)) - goto fail; + acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL); + status = PTR_ERR(acl); + if (IS_ERR(acl)) + goto out; } status = __nfs3_proc_setacls(inode, acl, dfacl); - posix_acl_release(alloc); +out: + if (acl != orig) + posix_acl_release(acl); + if (dfacl != orig) + posix_acl_release(dfacl); return status; - -fail: - return PTR_ERR(alloc); } const struct xattr_handler *nfs3_xattr_handlers[] = { base-commit: ae83d0b416db002fe95601e7f97f64b59514d936
On Mon, 2020-04-20 at 14:51 +0200, Andreas Gruenbacher wrote: > Am Mo., 20. Apr. 2020 um 14:15 Uhr schrieb Trond Myklebust < > trondmy@hammerspace.com>: > > I don't really see any alternative to adding a 'dfalloc' to track > > the > > allocated dfacl separately. > > Something like the attached patch should work as well. > > Thanks, > Andreas > > --- > fs/nfs/nfs3acl.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c > index c5c3fc6e6c60..f1581f11c220 100644 > --- a/fs/nfs/nfs3acl.c > +++ b/fs/nfs/nfs3acl.c > @@ -253,37 +253,41 @@ int nfs3_proc_setacls(struct inode *inode, > struct posix_acl *acl, > > int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int > type) > { > - struct posix_acl *alloc = NULL, *dfacl = NULL; > + struct posix_acl *orig = acl, *dfacl = NULL; > int status; > > if (S_ISDIR(inode->i_mode)) { > switch(type) { > case ACL_TYPE_ACCESS: > - alloc = dfacl = get_acl(inode, > ACL_TYPE_DEFAULT); > - if (IS_ERR(alloc)) > - goto fail; > + dfacl = get_acl(inode, ACL_TYPE_DEFAULT); > + status = PTR_ERR(dfacl); > + if (IS_ERR(dfacl)) > + goto out; > break; > > case ACL_TYPE_DEFAULT: > dfacl = acl; > - alloc = acl = get_acl(inode, ACL_TYPE_ACCESS); > - if (IS_ERR(alloc)) > - goto fail; > + acl = get_acl(inode, ACL_TYPE_ACCESS); > + status = PTR_ERR(acl); > + if (IS_ERR(acl)) > + goto out; > break; > } > } > > if (acl == NULL) { > - alloc = acl = posix_acl_from_mode(inode->i_mode, > GFP_KERNEL); > - if (IS_ERR(alloc)) > - goto fail; > + acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL); > + status = PTR_ERR(acl); > + if (IS_ERR(acl)) > + goto out; > } > status = __nfs3_proc_setacls(inode, acl, dfacl); > - posix_acl_release(alloc); > +out: > + if (acl != orig) > + posix_acl_release(acl); > + if (dfacl != orig) > + posix_acl_release(dfacl); > return status; > - > -fail: > - return PTR_ERR(alloc); > } > > const struct xattr_handler *nfs3_xattr_handlers[] = { > > base-commit: ae83d0b416db002fe95601e7f97f64b59514d936 Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl) triggers, shouldn't it?
On Mon, Apr 20, 2020 at 2:59 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl) > triggers, shouldn't it? Yes, checks missings. But you get the idea. Thanks, Andreas
On Mon, 2020-04-20 at 15:04 +0200, Andreas Gruenbacher wrote: > On Mon, Apr 20, 2020 at 2:59 PM Trond Myklebust < > trondmy@hammerspace.com> wrote: > > Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl) > > triggers, shouldn't it? > > Yes, checks missings. But you get the idea. > Fair enough
diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c index c5c3fc6e6c60..b5c41bcca8cf 100644 --- a/fs/nfs/nfs3acl.c +++ b/fs/nfs/nfs3acl.c @@ -274,6 +274,8 @@ int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int type) } if (acl == NULL) { + if (alloc) + posix_acl_release(alloc); alloc = acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL); if (IS_ERR(alloc)) goto fail;