[PATCH] ext4: fix invalid free tracking in ext4_xattr_move_to_block()

[PATCH] ext4: fix invalid free tracking in ext4_xattr_move_to_block()

[Theodore Ts'o]

In ext4_xattr_move_to_block(), the value of the extended attribute
which we need to move to an external block may be allocated by
kvmalloc() if the value is stored in an external inode.  So at the end
of the function the code tried to check if this was the case by
testing entry->e_value_inum.

However, at this point, the pointer to the xattr entry is no longer
valid, because it was removed from the original location where it had
been stored.  So we could end up calling kvfree() on a pointer which
was not allocated by kvmalloc(); or we could also potentially leak
memory by not freeing the buffer when it should be freed.  Fix this by
storing whether it should be freed in a separate variable.

Link: https://syzkaller.appspot.com/bug?id=5c2aee8256e30b55ccf57312c16d88417adbd5e1
Link: https://syzkaller.appspot.com/bug?id=41a6b5d4917c0412eb3b3c3c604965bed7d7420b
Reported-by: syzbot+64b645917ce07d89bde5@syzkaller.appspotmail.com
Reported-by: syzbot+0d042627c4f2ad332195@syzkaller.appspotmail.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/xattr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 767454d74cd6..e33a323faf3c 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2615,6 +2615,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
 		.in_inode = !!entry->e_value_inum,
 	};
 	struct ext4_xattr_ibody_header *header = IHDR(inode, raw_inode);
+	int needs_kvfree = 0;
 	int error;
 
 	is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
@@ -2637,7 +2638,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
 			error = -ENOMEM;
 			goto out;
 		}
-
+		needs_kvfree = 1;
 		error = ext4_xattr_inode_get(inode, entry, buffer, value_size);
 		if (error)
 			goto out;
@@ -2676,7 +2677,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
 
 out:
 	kfree(b_entry_name);
-	if (entry->e_value_inum && buffer)
+	if (needs_kvfree && buffer)
 		kvfree(buffer);
 	if (is)
 		brelse(is->iloc.bh);
-- 
2.31.0

Re: [PATCH] ext4: fix invalid free tracking in ext4_xattr_move_to_block()

[Jan Kara]

On Sun 30-04-23 12:04:26, Theodore Ts'o wrote:
> In ext4_xattr_move_to_block(), the value of the extended attribute
> which we need to move to an external block may be allocated by
> kvmalloc() if the value is stored in an external inode.  So at the end
> of the function the code tried to check if this was the case by
> testing entry->e_value_inum.
> 
> However, at this point, the pointer to the xattr entry is no longer
> valid, because it was removed from the original location where it had
> been stored.  So we could end up calling kvfree() on a pointer which
> was not allocated by kvmalloc(); or we could also potentially leak
> memory by not freeing the buffer when it should be freed.  Fix this by
> storing whether it should be freed in a separate variable.
> 
> Link: https://syzkaller.appspot.com/bug?id=5c2aee8256e30b55ccf57312c16d88417adbd5e1
> Link: https://syzkaller.appspot.com/bug?id=41a6b5d4917c0412eb3b3c3c604965bed7d7420b
> Reported-by: syzbot+64b645917ce07d89bde5@syzkaller.appspotmail.com
> Reported-by: syzbot+0d042627c4f2ad332195@syzkaller.appspotmail.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Good spotting. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/xattr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 767454d74cd6..e33a323faf3c 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2615,6 +2615,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  		.in_inode = !!entry->e_value_inum,
>  	};
>  	struct ext4_xattr_ibody_header *header = IHDR(inode, raw_inode);
> +	int needs_kvfree = 0;
>  	int error;
>  
>  	is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
> @@ -2637,7 +2638,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  			error = -ENOMEM;
>  			goto out;
>  		}
> -
> +		needs_kvfree = 1;
>  		error = ext4_xattr_inode_get(inode, entry, buffer, value_size);
>  		if (error)
>  			goto out;
> @@ -2676,7 +2677,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  
>  out:
>  	kfree(b_entry_name);
> -	if (entry->e_value_inum && buffer)
> +	if (needs_kvfree && buffer)
>  		kvfree(buffer);
>  	if (is)
>  		brelse(is->iloc.bh);
> -- 
> 2.31.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: fix invalid free tracking in ext4_xattr_move_to_block()

[Tudor Ambarus]

On 4/30/23 17:04, Theodore Ts'o wrote:
> In ext4_xattr_move_to_block(), the value of the extended attribute
> which we need to move to an external block may be allocated by
> kvmalloc() if the value is stored in an external inode.  So at the end
> of the function the code tried to check if this was the case by
> testing entry->e_value_inum.
> 
> However, at this point, the pointer to the xattr entry is no longer
> valid, because it was removed from the original location where it had
> been stored.  So we could end up calling kvfree() on a pointer which
> was not allocated by kvmalloc(); or we could also potentially leak
> memory by not freeing the buffer when it should be freed.  Fix this by
> storing whether it should be freed in a separate variable.
> 
> Link: https://syzkaller.appspot.com/bug?id=5c2aee8256e30b55ccf57312c16d88417adbd5e1
> Link: https://syzkaller.appspot.com/bug?id=41a6b5d4917c0412eb3b3c3c604965bed7d7420b
> Reported-by: syzbot+64b645917ce07d89bde5@syzkaller.appspotmail.com
> Reported-by: syzbot+0d042627c4f2ad332195@syzkaller.appspotmail.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  fs/ext4/xattr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 767454d74cd6..e33a323faf3c 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2615,6 +2615,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  		.in_inode = !!entry->e_value_inum,
>  	};
>  	struct ext4_xattr_ibody_header *header = IHDR(inode, raw_inode);
> +	int needs_kvfree = 0;
>  	int error;
>  
>  	is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
> @@ -2637,7 +2638,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  			error = -ENOMEM;
>  			goto out;
>  		}
> -
> +		needs_kvfree = 1;
>  		error = ext4_xattr_inode_get(inode, entry, buffer, value_size);
>  		if (error)
>  			goto out;
> @@ -2676,7 +2677,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  
>  out:
>  	kfree(b_entry_name);
> -	if (entry->e_value_inum && buffer)
> +	if (needs_kvfree && buffer)

If buffer is null, no operation should be performed, so we may get rid
of the buffer check.

We should also add the Fixes tag and Cc: stable@vger.kernel.org, as the
blamed commit fixes another bug and it was backported to 4.14+, thus
this one needs to be backported as well.

Fixes: 1e9d62d25281 ("ext4: optimize ea_inode block expansion")

Anyway:
Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Cheers,
ta
>  		kvfree(buffer);
>  	if (is)
>  		brelse(is->iloc.bh);

Re: [PATCH] ext4: fix invalid free tracking in ext4_xattr_move_to_block()

[Tudor Ambarus]

On 5/8/23 11:20, Tudor Ambarus wrote:
> 
> 
> On 4/30/23 17:04, Theodore Ts'o wrote:
>> In ext4_xattr_move_to_block(), the value of the extended attribute
>> which we need to move to an external block may be allocated by
>> kvmalloc() if the value is stored in an external inode.  So at the end
>> of the function the code tried to check if this was the case by
>> testing entry->e_value_inum.
>>
>> However, at this point, the pointer to the xattr entry is no longer
>> valid, because it was removed from the original location where it had
>> been stored.  So we could end up calling kvfree() on a pointer which
>> was not allocated by kvmalloc(); or we could also potentially leak
>> memory by not freeing the buffer when it should be freed.  Fix this by
>> storing whether it should be freed in a separate variable.
>>
>> Link: https://syzkaller.appspot.com/bug?id=5c2aee8256e30b55ccf57312c16d88417adbd5e1
>> Link: https://syzkaller.appspot.com/bug?id=41a6b5d4917c0412eb3b3c3c604965bed7d7420b
>> Reported-by: syzbot+64b645917ce07d89bde5@syzkaller.appspotmail.com
>> Reported-by: syzbot+0d042627c4f2ad332195@syzkaller.appspotmail.com
>> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
>> ---
>>  fs/ext4/xattr.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>> index 767454d74cd6..e33a323faf3c 100644
>> --- a/fs/ext4/xattr.c
>> +++ b/fs/ext4/xattr.c
>> @@ -2615,6 +2615,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>>  		.in_inode = !!entry->e_value_inum,
>>  	};
>>  	struct ext4_xattr_ibody_header *header = IHDR(inode, raw_inode);
>> +	int needs_kvfree = 0;
>>  	int error;
>>  
>>  	is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
>> @@ -2637,7 +2638,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>>  			error = -ENOMEM;
>>  			goto out;
>>  		}
>> -
>> +		needs_kvfree = 1;
>>  		error = ext4_xattr_inode_get(inode, entry, buffer, value_size);
>>  		if (error)
>>  			goto out;
>> @@ -2676,7 +2677,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>>  
>>  out:
>>  	kfree(b_entry_name);
>> -	if (entry->e_value_inum && buffer)
>> +	if (needs_kvfree && buffer)
> 
> If buffer is null, no operation should be performed, so we may get rid
> of the buffer check.
> 

I meant that if buffer is null no operation is performed for
kvfree(buffer). Sent a patch on top of yours at:

https://lore.kernel.org/linux-ext4/20230508151337.79304-1-tudor.ambarus@linaro.org/T/#u

Cheers,
ta

> We should also add the Fixes tag and Cc: stable@vger.kernel.org, as the
> blamed commit fixes another bug and it was backported to 4.14+, thus
> this one needs to be backported as well.
> 
> Fixes: 1e9d62d25281 ("ext4: optimize ea_inode block expansion")
> 
> Anyway:
> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> 
> Cheers,
> ta
>>  		kvfree(buffer);
>>  	if (is)
>>  		brelse(is->iloc.bh);