[Devel] [PATCH] ext4: release leaked posix acl in ext4_xattr_set_acl
Posix acl is used to convert of an extended attribute, provided by user to ext4 attributes. In particular to I-mode in case of ACL_TYPE_ACCESS request. IOW, this object is allocated, used for convertion, not stored anywhere and must be freed. However posix_acl_update_mode() can zerofy the pointer to support ext4_set_acl() logic, but then the object is leaked. So, fix it by releasing new temporary pointer with the same value instead of acl pointer. https://jira.sw.ru/browse/PSBM-81384 Signed-off-by: Stanislav Kinsburskiy --- fs/ext4/acl.c |8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c index 917e819..2640d7b 100644 --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c @@ -403,7 +403,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, const void *value, { struct inode *inode = dentry->d_inode; handle_t *handle; - struct posix_acl *acl; + struct posix_acl *acl, *tmp; int error, retries = 0; int update_mode = 0; umode_t mode = inode->i_mode; @@ -416,7 +416,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, const void *value, return -EPERM; if (value) { - acl = posix_acl_from_xattr(&init_user_ns, value, size); + acl = tmp = posix_acl_from_xattr(&init_user_ns, value, size); if (IS_ERR(acl)) return PTR_ERR(acl); else if (acl) { @@ -425,7 +425,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, const void *value, goto release_and_out; } } else - acl = NULL; + acl = tmp = NULL; retry: handle = ext4_journal_start(inode, EXT4_HT_XATTR, @@ -452,7 +452,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char *name, const void *value, goto retry; release_and_out: - posix_acl_release(acl); + posix_acl_release(tmp); return error; } ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] ext4: release leaked posix acl in ext4_xattr_set_acl
Stanislav Kinsburskiy writes: > Posix acl is used to convert of an extended attribute, provided by user to > ext4 attributes. In particular to I-mode in case of ACL_TYPE_ACCESS request. > IOW, this object is allocated, used for convertion, not stored anywhere and > must be freed. > However posix_acl_update_mode() can zerofy the pointer to support > ext4_set_acl() logic, but then the object is leaked. > So, fix it by releasing new temporary pointer with the same value instead of > acl pointer. So you are telling me that: ext4_xattr_set_acl L1 acl = posix_acl_from_xattr L2 -> ext4_set_acl(handle, inode, type, acl) L3->posix_acl_update_mode(inode, &inode->i_mode, &acl) *acl = NULL; You are saying that instruction above can affect value at L1? HOW? acl passed to ext4_set_acl() by value, so posix_acl_update_mode() can affect value only in L2 and L3 but not L1. Stas, have you drunk a lousy beer today? > > https://jira.sw.ru/browse/PSBM-81384 > > Signed-off-by: Stanislav Kinsburskiy > --- > fs/ext4/acl.c |8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c > index 917e819..2640d7b 100644 > --- a/fs/ext4/acl.c > +++ b/fs/ext4/acl.c > @@ -403,7 +403,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char > *name, const void *value, > { > struct inode *inode = dentry->d_inode; > handle_t *handle; > - struct posix_acl *acl; > + struct posix_acl *acl, *tmp; > int error, retries = 0; > int update_mode = 0; > umode_t mode = inode->i_mode; > @@ -416,7 +416,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char > *name, const void *value, > return -EPERM; > > if (value) { > - acl = posix_acl_from_xattr(&init_user_ns, value, size); > + acl = tmp = posix_acl_from_xattr(&init_user_ns, value, size); > if (IS_ERR(acl)) > return PTR_ERR(acl); > else if (acl) { > @@ -425,7 +425,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char > *name, const void *value, > goto release_and_out; > } > } else > - acl = NULL; > + acl = tmp = NULL; > > retry: > handle = ext4_journal_start(inode, EXT4_HT_XATTR, > @@ -452,7 +452,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char > *name, const void *value, > goto retry; > > release_and_out: > - posix_acl_release(acl); > + posix_acl_release(tmp); > return error; > } > signature.asc Description: PGP signature ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [PATCH] ext4: release leaked posix acl in ext4_xattr_set_acl
07.02.2018 14:51, Dmitry Monakhov пишет: > Stanislav Kinsburskiy writes: > >> Posix acl is used to convert of an extended attribute, provided by user to >> ext4 attributes. In particular to I-mode in case of ACL_TYPE_ACCESS request. >> IOW, this object is allocated, used for convertion, not stored anywhere and >> must be freed. >> However posix_acl_update_mode() can zerofy the pointer to support >> ext4_set_acl() logic, but then the object is leaked. >> So, fix it by releasing new temporary pointer with the same value instead of >> acl pointer. > So you are telling me that: > ext4_xattr_set_acl > L1 acl = posix_acl_from_xattr > L2 -> ext4_set_acl(handle, inode, type, acl) > L3->posix_acl_update_mode(inode, &inode->i_mode, &acl) > *acl = NULL; > You are saying that instruction above can affect value at L1? > HOW? acl passed to ext4_set_acl() by value, so > posix_acl_update_mode() can affect value only in L2 and L3 but not L1. > > Stas, have you drunk a lousy beer today? OMG, Dmitry... Looks like storage work clouded your mind and you are looking to some other sources. There is no "L3" in the bug. Because posix_acl_update_mode() in called from ext4_xattr_set_acl() (i.e on "L2"). Thus pointer is set to NULL on "L2" and released on "L1". IOW, pointer is leaked. >> >> https://jira.sw.ru/browse/PSBM-81384 >> >> Signed-off-by: Stanislav Kinsburskiy >> --- >> fs/ext4/acl.c |8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c >> index 917e819..2640d7b 100644 >> --- a/fs/ext4/acl.c >> +++ b/fs/ext4/acl.c >> @@ -403,7 +403,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char >> *name, const void *value, >> { >> struct inode *inode = dentry->d_inode; >> handle_t *handle; >> -struct posix_acl *acl; >> +struct posix_acl *acl, *tmp; >> int error, retries = 0; >> int update_mode = 0; >> umode_t mode = inode->i_mode; >> @@ -416,7 +416,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char >> *name, const void *value, >> return -EPERM; >> >> if (value) { >> -acl = posix_acl_from_xattr(&init_user_ns, value, size); >> +acl = tmp = posix_acl_from_xattr(&init_user_ns, value, size); >> if (IS_ERR(acl)) >> return PTR_ERR(acl); >> else if (acl) { >> @@ -425,7 +425,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char >> *name, const void *value, >> goto release_and_out; >> } >> } else >> -acl = NULL; >> +acl = tmp = NULL; >> >> retry: >> handle = ext4_journal_start(inode, EXT4_HT_XATTR, >> @@ -452,7 +452,7 @@ ext4_xattr_set_acl(struct dentry *dentry, const char >> *name, const void *value, >> goto retry; >> >> release_and_out: >> -posix_acl_release(acl); >> +posix_acl_release(tmp); >> return error; >> } >> ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel