| 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;
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(+)