ext4: release leaked posix acl in ext4_xattr_set_acl

Submitted by Stanislav Kinsburskiy on Feb. 7, 2018, 1:08 p.m.

Details

Message ID 20180207130559.18342.46891.stgit@localhost.localdomain
State New
Series "ext4: release leaked posix acl in ext4_xattr_set_acl"
Headers show

Commit Message

Stanislav Kinsburskiy Feb. 7, 2018, 1:08 p.m.
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 <skinsbursky@virtuozzo.com>
---
 fs/ext4/acl.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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;
 }
 

Comments

Dmitry Monakhov Feb. 7, 2018, 1:51 p.m.
Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> 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 <skinsbursky@virtuozzo.com>
> ---
>  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;
>  }
>
Stanislav Kinsburskiy Feb. 7, 2018, 2:27 p.m.
07.02.2018 14:51, Dmitry Monakhov пишет:
> Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> 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 <skinsbursky@virtuozzo.com>
>> ---
>>  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;
>>  }
>>