[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
