[PATCH v2] ext2: fix datatype of block number in ext2_xattr_set2()

[PATCH v2] ext2: fix datatype of block number in ext2_xattr_set2()

[Georg Ottinger]

I run a small server that uses external hard drives for backups. The
backup software I use uses ext2 filesystems with 4KiB block size and
the server is running SELinux and therefore relies on xattr. I recently
upgraded the hard drives from 4TB to 12TB models. I noticed that after
transferring some TBs I got a filesystem error "Freeing blocks not in
datazone - block = 18446744071529317386, count = 1" and the backup
process stopped. Trying to fix the fs with e2fsck resulted in a
completely corrupted fs. The error probably came from ext2_free_blocks(),
and because of the large number 18e19 this problem immediately looked
like some kind of integer overflow. Whereas the 4TB fs was about 1e9
blocks, the new 12TB is about 3e9 blocks. So, searching the ext2 code,
I came across the line in fs/ext2/xattr.c:745 where ext2_new_block()
is called and the resulting block number is stored in the variable block
as an int datatype. If a block with a block number greater than
INT32_MAX is returned, this variable overflows and the call to
sb_getblk() at line fs/ext2/xattr.c:750 fails, then the call to
ext2_free_blocks() produces the error.

Signed-off-by: Georg Ottinger <g.ottinger@gmx.at>
---
 fs/ext2/xattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 8906ba479..89517937d 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -742,10 +742,10 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
 			/* We need to allocate a new block */
 			ext2_fsblk_t goal = ext2_group_first_block_no(sb,
 						EXT2_I(inode)->i_block_group);
-			int block = ext2_new_block(inode, goal, &error);
+			ext2_fsblk_t block = ext2_new_block(inode, goal, &error);
 			if (error)
 				goto cleanup;
-			ea_idebug(inode, "creating block %d", block);
+			ea_idebug(inode, "creating block %lu", block);

 			new_bh = sb_getblk(sb, block);
 			if (unlikely(!new_bh)) {
--
2.17.1

Re: [PATCH v2] ext2: fix datatype of block number in ext2_xattr_set2()

[Georg Ottinger]

I missed the proper format string for the debug message.

answering Andreas question - I did check the remaining calls to
ext2_new_block(), ext2_new_blocks() and ext2_free_blocks() within the
ext2 directory - here the block argument is either unsigned long or
ext2_fsblk_t (which is a typedef to unsigend long) - However I want to
mention that the use of unsigned long / ext2_fsblk_t is inconsistent. I
guess that ext2_fsblk_t should be the prefered data type.

Concerning the fs corruption - thanks Jan for your input.

The server is an old Centos6 / RHEL6 Machine, and as a workaround - i
resized the partition to 8180GB - Unfortunately I am unsure if I find
time to investigate this issue further - I just did a quick look at the
kernel source and the Bug was kind of obvious ...


On 16.08.23 07:21, Georg Ottinger wrote:
> I run a small server that uses external hard drives for backups. The
> backup software I use uses ext2 filesystems with 4KiB block size and
> the server is running SELinux and therefore relies on xattr. I recently
> upgraded the hard drives from 4TB to 12TB models. I noticed that after
> transferring some TBs I got a filesystem error "Freeing blocks not in
> datazone - block = 18446744071529317386, count = 1" and the backup
> process stopped. Trying to fix the fs with e2fsck resulted in a
> completely corrupted fs. The error probably came from ext2_free_blocks(),
> and because of the large number 18e19 this problem immediately looked
> like some kind of integer overflow. Whereas the 4TB fs was about 1e9
> blocks, the new 12TB is about 3e9 blocks. So, searching the ext2 code,
> I came across the line in fs/ext2/xattr.c:745 where ext2_new_block()
> is called and the resulting block number is stored in the variable block
> as an int datatype. If a block with a block number greater than
> INT32_MAX is returned, this variable overflows and the call to
> sb_getblk() at line fs/ext2/xattr.c:750 fails, then the call to
> ext2_free_blocks() produces the error.
>
> Signed-off-by: Georg Ottinger <g.ottinger@gmx.at>
> ---
>   fs/ext2/xattr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 8906ba479..89517937d 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -742,10 +742,10 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
>   			/* We need to allocate a new block */
>   			ext2_fsblk_t goal = ext2_group_first_block_no(sb,
>   						EXT2_I(inode)->i_block_group);
> -			int block = ext2_new_block(inode, goal, &error);
> +			ext2_fsblk_t block = ext2_new_block(inode, goal, &error);
>   			if (error)
>   				goto cleanup;
> -			ea_idebug(inode, "creating block %d", block);
> +			ea_idebug(inode, "creating block %lu", block);
>
>   			new_bh = sb_getblk(sb, block);
>   			if (unlikely(!new_bh)) {
> --
> 2.17.1
>

Re: [PATCH v2] ext2: fix datatype of block number in ext2_xattr_set2()

[Jan Kara]

On Wed 16-08-23 07:31:34, Georg Ottinger wrote:
> I missed the proper format string for the debug message.

Thanks. I've fixed up the patch in my tree.

> answering Andreas question - I did check the remaining calls to
> ext2_new_block(), ext2_new_blocks() and ext2_free_blocks() within the
> ext2 directory - here the block argument is either unsigned long or
> ext2_fsblk_t (which is a typedef to unsigend long) - However I want to
> mention that the use of unsigned long / ext2_fsblk_t is inconsistent. I
> guess that ext2_fsblk_t should be the prefered data type.

Yes, that's correct. We should be using ext2_fsblk_t all over the place. In
fact unsigned long is also a questionable type. On disk the block number is
u32, so unsigned long is pointlessly big on 64-bit archs and just using u32
as ext2_fsblk_t would make more sense. But then it's possible there are
some overflows in the code currently hidden by the fact that most of the
testing happens on 64-bit where long is 64-bit. So the switch would need
somewhat careful review.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR