lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Wed, 12 Nov 2014 13:30:45 -0800
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Xiaoguang Wang <wangxg.fnst@...fujitsu.com>
Cc:	linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH 3/4] e2fsprogs/tune2fs: rewrite metadata checksums when
 resizing inode size

On Tue, Nov 11, 2014 at 05:08:42PM +0800, Xiaoguang Wang wrote:
> Hi,
> 
> On 11/11/2014 04:33 PM, Darrick J. Wong wrote:
> > On Tue, Nov 11, 2014 at 02:59:28PM +0800, Xiaoguang Wang wrote:
> >> When we use tune2fs -I new_ino_size to change inode size, if everything is OK,
> >> the corresponding ext4_group_desc.bg_free_blocks_count will be decreased, so
> >> obviously, we need to re-compute the group descriptor checksums, fix this. If
> >> not doing this, mount operation will fail.
> >>
> >> Meanwhile, the patch will trigger an existing memory write overflow, which will
> >> casue segfault, please see the next patch.
> >>
> >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@...fujitsu.com>
> >> ---
> >>  misc/tune2fs.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> >> index 065b483..91dc7c1 100644
> >> --- a/misc/tune2fs.c
> >> +++ b/misc/tune2fs.c
> >> @@ -2908,8 +2908,7 @@ retry_open:
> >>  				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> >>  			rewrite_checksums = 1;
> >>  	}
> >> -	if (rewrite_checksums)
> >> -		rewrite_metadata_checksums(fs);
> >> +
> >>  	if (I_flag) {
> >>  		if (mount_flags & EXT2_MF_MOUNTED) {
> >>  			fputs(_("The inode size may only be "
> >> @@ -2935,6 +2934,7 @@ retry_open:
> >>  		if (resize_inode(fs, new_inode_size) == 0) {
> >>  			printf(_("Setting inode size %lu\n"),
> >>  							new_inode_size);
> >> +			rewrite_checksums = 1;
> > 
> > rewrite_metadata_checksums() was designed to rewrite checksums on every
> > metadata object in the filesystem (extents, ACLs, directory blocks), which
> > I think does more work than necessary for your use case (if I'm reading
> > this correctly) of simply rewriting the block group descriptor checksums.
> > Would it suffice to call ext2fs_group_desc_csum_set() when setting
> > bg_free_blocks_count?  I think this could be done in
> > ext2fs_calculate_summary_stats().
> 
> For a new created file system, I think ext2fs_group_desc_csum_set() would be
> sufficient, because the needed blocks(used for new inode table, they must be
> continuous) will be free. But for a file system, if it already has many
> directories and files, these blocks may be in use, then we need to replace
> these blocks with other blocks, so the ACLs and extent tree may also be
> modified, see inode_scan_and_fix(), so I think here we need a
> rewrite_metadata_checksums(), in other words, it's the safest :) I'm still
> new to ext4(learning the code now), so I may also miss something, thanks!

The checksums for extent blocks, ACLs, and directories don't care about the LBA
of the block, which means that they can be moved around the FS without having
to rewrite the checksum.  I still think you can get away with just:

for (dgrp_t i = 0; i < fs->group_desc_count; i++)
	ext2fs_group_desc_csum_set(fs, i);

to fix your problem.  That said, do you have a test case you could send me?
I'm having trouble reproducing the symptoms on the -next branch.  1.42.12 can
reproduce it reliably... but the funny thing is that 1.42.* doesn't know about
metadata_csum at all.

--D

> 
> Regards,
> Xiaoguang Wang
> 
>  
> 
> > 
> > --D
> > 
> >>  		} else {
> >>  			printf("%s", _("Failed to change inode size\n"));
> >>  			rc = 1;
> >> @@ -2942,6 +2942,9 @@ retry_open:
> >>  		}
> >>  	}
> >>  
> >> +	if (rewrite_checksums)
> >> +		rewrite_metadata_checksums(fs);
> >> +
> >>  	if (l_flag)
> >>  		list_super(sb);
> >>  	if (stride_set) {
> >> -- 
> >> 1.8.2.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> >> the body of a message to majordomo@...r.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > .
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists