* [PATCH 0/2] Fixes for dir_index support in libext2fs @ 2020-03-30 9:09 Jan Kara 2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara 2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara 0 siblings, 2 replies; 14+ messages in thread From: Jan Kara @ 2020-03-30 9:09 UTC (permalink / raw) To: Ted Tso; +Cc: linux-ext4, Jan Kara Hello, here are two small fixes that I've spotted in the new support for adding entries into indexed directories. Honza ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] ext2fs: Fix error checking in dx_link() 2020-03-30 9:09 [PATCH 0/2] Fixes for dir_index support in libext2fs Jan Kara @ 2020-03-30 9:09 ` Jan Kara 2020-03-30 13:24 ` Lukas Czerner 2020-04-10 3:52 ` Theodore Y. Ts'o 2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara 1 sibling, 2 replies; 14+ messages in thread From: Jan Kara @ 2020-03-30 9:09 UTC (permalink / raw) To: Ted Tso; +Cc: linux-ext4, Jan Kara dx_lookup() uses errcode_t return values. As such anything non-zero is an error, not values less than zero. Fix the error checking to avoid crashes on corrupted filesystems. Signed-off-by: Jan Kara <jack@suse.cz> --- lib/ext2fs/link.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c index 6f523aee718c..7b5bb022117c 100644 --- a/lib/ext2fs/link.c +++ b/lib/ext2fs/link.c @@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir, dx_info.namelen = strlen(name); again: retval = dx_lookup(fs, dir, diri, &dx_info); - if (retval < 0) + if (retval) goto free_buf; retval = add_dirent_to_buf(fs, -- 2.16.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link() 2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara @ 2020-03-30 13:24 ` Lukas Czerner 2020-03-30 13:48 ` Lukas Czerner 2020-04-10 3:52 ` Theodore Y. Ts'o 1 sibling, 1 reply; 14+ messages in thread From: Lukas Czerner @ 2020-03-30 13:24 UTC (permalink / raw) To: Jan Kara; +Cc: Ted Tso, linux-ext4 On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote: > dx_lookup() uses errcode_t return values. As such anything non-zero is > an error, not values less than zero. Fix the error checking to avoid > crashes on corrupted filesystems. Looks good, thanks. Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > lib/ext2fs/link.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c > index 6f523aee718c..7b5bb022117c 100644 > --- a/lib/ext2fs/link.c > +++ b/lib/ext2fs/link.c > @@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir, > dx_info.namelen = strlen(name); > again: > retval = dx_lookup(fs, dir, diri, &dx_info); > - if (retval < 0) > + if (retval) > goto free_buf; > > retval = add_dirent_to_buf(fs, > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link() 2020-03-30 13:24 ` Lukas Czerner @ 2020-03-30 13:48 ` Lukas Czerner 2020-03-30 14:55 ` Jan Kara 0 siblings, 1 reply; 14+ messages in thread From: Lukas Czerner @ 2020-03-30 13:48 UTC (permalink / raw) To: Jan Kara; +Cc: Ted Tso, linux-ext4 On Mon, Mar 30, 2020 at 03:24:40PM +0200, Lukas Czerner wrote: > On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote: > > dx_lookup() uses errcode_t return values. As such anything non-zero is > > an error, not values less than zero. Fix the error checking to avoid > > crashes on corrupted filesystems. > > Looks good, thanks. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> of course that should be Reviewed-by: Lukas Czerner <lczerner@redhat.com> -Lukas > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > lib/ext2fs/link.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c > > index 6f523aee718c..7b5bb022117c 100644 > > --- a/lib/ext2fs/link.c > > +++ b/lib/ext2fs/link.c > > @@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir, > > dx_info.namelen = strlen(name); > > again: > > retval = dx_lookup(fs, dir, diri, &dx_info); > > - if (retval < 0) > > + if (retval) > > goto free_buf; > > > > retval = add_dirent_to_buf(fs, > > -- > > 2.16.4 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link() 2020-03-30 13:48 ` Lukas Czerner @ 2020-03-30 14:55 ` Jan Kara 0 siblings, 0 replies; 14+ messages in thread From: Jan Kara @ 2020-03-30 14:55 UTC (permalink / raw) To: Lukas Czerner; +Cc: Jan Kara, Ted Tso, linux-ext4 On Mon 30-03-20 15:48:53, Lukas Czerner wrote: > On Mon, Mar 30, 2020 at 03:24:40PM +0200, Lukas Czerner wrote: > > On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote: > > > dx_lookup() uses errcode_t return values. As such anything non-zero is > > > an error, not values less than zero. Fix the error checking to avoid > > > crashes on corrupted filesystems. > > > > Looks good, thanks. > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > of course that should be > > Reviewed-by: Lukas Czerner <lczerner@redhat.com> Thanks for review! Honza > > -Lukas > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > lib/ext2fs/link.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c > > > index 6f523aee718c..7b5bb022117c 100644 > > > --- a/lib/ext2fs/link.c > > > +++ b/lib/ext2fs/link.c > > > @@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir, > > > dx_info.namelen = strlen(name); > > > again: > > > retval = dx_lookup(fs, dir, diri, &dx_info); > > > - if (retval < 0) > > > + if (retval) > > > goto free_buf; > > > > > > retval = add_dirent_to_buf(fs, > > > -- > > > 2.16.4 > > > > > > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link() 2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara 2020-03-30 13:24 ` Lukas Czerner @ 2020-04-10 3:52 ` Theodore Y. Ts'o 1 sibling, 0 replies; 14+ messages in thread From: Theodore Y. Ts'o @ 2020-04-10 3:52 UTC (permalink / raw) To: Jan Kara; +Cc: linux-ext4 On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote: > dx_lookup() uses errcode_t return values. As such anything non-zero is > an error, not values less than zero. Fix the error checking to avoid > crashes on corrupted filesystems. > > Signed-off-by: Jan Kara <jack@suse.cz> Applied, thanks. - Ted ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-03-30 9:09 [PATCH 0/2] Fixes for dir_index support in libext2fs Jan Kara 2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara @ 2020-03-30 9:09 ` Jan Kara 2020-03-30 13:27 ` Lukas Czerner 2020-04-10 4:32 ` Theodore Y. Ts'o 1 sibling, 2 replies; 14+ messages in thread From: Jan Kara @ 2020-03-30 9:09 UTC (permalink / raw) To: Ted Tso; +Cc: linux-ext4, Jan Kara There is an off-by-one error in dx_grow_tree() when checking whether we can add another level to the tree. Thus we can grow tree too much leading to possible crashes in the library or corrupted filesystem. Fix the bug. Signed-off-by: Jan Kara <jack@suse.cz> --- lib/ext2fs/link.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c index 7b5bb022117c..469eea8cd06d 100644 --- a/lib/ext2fs/link.c +++ b/lib/ext2fs/link.c @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir, ext2fs_le16_to_cpu(info->frames[i].head->limit)) break; /* Need to grow tree depth? */ - if (i < 0 && info->levels > ext2_dir_htree_level(fs)) + if (i < 0 && info->levels >= ext2_dir_htree_level(fs)) return EXT2_ET_DIR_NO_SPACE; lblk = size / fs->blocksize; size += fs->blocksize; -- 2.16.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara @ 2020-03-30 13:27 ` Lukas Czerner 2020-03-30 14:55 ` Jan Kara 2020-04-10 4:32 ` Theodore Y. Ts'o 1 sibling, 1 reply; 14+ messages in thread From: Lukas Czerner @ 2020-03-30 13:27 UTC (permalink / raw) To: Jan Kara; +Cc: Ted Tso, linux-ext4 On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote: > There is an off-by-one error in dx_grow_tree() when checking whether we > can add another level to the tree. Thus we can grow tree too much > leading to possible crashes in the library or corrupted filesystem. Fix > the bug. Looks good, thanks Reviewed-by: Lukas Czerner <lczerner@redhat.com> Don't we have basically the same off-by-one in e2fsck/pass1.c handle_htree() ? if ((root->indirect_levels > ext2_dir_htree_level(fs)) && fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) -Lukas > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > lib/ext2fs/link.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c > index 7b5bb022117c..469eea8cd06d 100644 > --- a/lib/ext2fs/link.c > +++ b/lib/ext2fs/link.c > @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir, > ext2fs_le16_to_cpu(info->frames[i].head->limit)) > break; > /* Need to grow tree depth? */ > - if (i < 0 && info->levels > ext2_dir_htree_level(fs)) > + if (i < 0 && info->levels >= ext2_dir_htree_level(fs)) > return EXT2_ET_DIR_NO_SPACE; > lblk = size / fs->blocksize; > size += fs->blocksize; > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-03-30 13:27 ` Lukas Czerner @ 2020-03-30 14:55 ` Jan Kara 2020-03-31 11:33 ` Lukas Czerner 0 siblings, 1 reply; 14+ messages in thread From: Jan Kara @ 2020-03-30 14:55 UTC (permalink / raw) To: Lukas Czerner; +Cc: Jan Kara, Ted Tso, linux-ext4 On Mon 30-03-20 15:27:12, Lukas Czerner wrote: > On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote: > > There is an off-by-one error in dx_grow_tree() when checking whether we > > can add another level to the tree. Thus we can grow tree too much > > leading to possible crashes in the library or corrupted filesystem. Fix > > the bug. > > Looks good, thanks > > Reviewed-by: Lukas Czerner <lczerner@redhat.com> > > Don't we have basically the same off-by-one in > e2fsck/pass1.c handle_htree() ? > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) I don't think so. It is indeed correct for root->indirect_levels to be equal to ext2_dir_htree_level(). However dx_grow_tree() is going to increase root->indirect_levels by 1 which is where tree would become invalid... Honza > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > lib/ext2fs/link.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c > > index 7b5bb022117c..469eea8cd06d 100644 > > --- a/lib/ext2fs/link.c > > +++ b/lib/ext2fs/link.c > > @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir, > > ext2fs_le16_to_cpu(info->frames[i].head->limit)) > > break; > > /* Need to grow tree depth? */ > > - if (i < 0 && info->levels > ext2_dir_htree_level(fs)) > > + if (i < 0 && info->levels >= ext2_dir_htree_level(fs)) > > return EXT2_ET_DIR_NO_SPACE; > > lblk = size / fs->blocksize; > > size += fs->blocksize; > > -- > > 2.16.4 > > > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-03-30 14:55 ` Jan Kara @ 2020-03-31 11:33 ` Lukas Czerner 2020-03-31 14:30 ` Jan Kara 0 siblings, 1 reply; 14+ messages in thread From: Lukas Czerner @ 2020-03-31 11:33 UTC (permalink / raw) To: Jan Kara; +Cc: Ted Tso, linux-ext4 On Mon, Mar 30, 2020 at 04:55:31PM +0200, Jan Kara wrote: > On Mon 30-03-20 15:27:12, Lukas Czerner wrote: > > On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote: > > > There is an off-by-one error in dx_grow_tree() when checking whether we > > > can add another level to the tree. Thus we can grow tree too much > > > leading to possible crashes in the library or corrupted filesystem. Fix > > > the bug. > > > > Looks good, thanks > > > > Reviewed-by: Lukas Czerner <lczerner@redhat.com> > > > > Don't we have basically the same off-by-one in > > e2fsck/pass1.c handle_htree() ? > > > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) > > I don't think so. It is indeed correct for root->indirect_levels to be > equal to ext2_dir_htree_level(). However dx_grow_tree() is going to > increase root->indirect_levels by 1 which is where tree would become > invalid... > > Honza Hmm, are you sure ? I think the names are very confusing root->indirect_levels is zero based, while ext2_dir_htree_level() returns the maximum number of levels (that is 3 by default). If I am right then indirect_levels must always be smaller then ext2_dir_htree_level() and that is how we use it everywhere else - the palce I am pointing out is an exception and I think it's a bug. Indeed it looks like the bug got introduced in 3f0cf647539970474be8f607017ca7eccfc2fbbe - if ((root->indirect_levels > 1) && + if ((root->indirect_levels > ext2_dir_htree_level(fs)) && Or am I missing something ? -Lukas > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > lib/ext2fs/link.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c > > > index 7b5bb022117c..469eea8cd06d 100644 > > > --- a/lib/ext2fs/link.c > > > +++ b/lib/ext2fs/link.c > > > @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir, > > > ext2fs_le16_to_cpu(info->frames[i].head->limit)) > > > break; > > > /* Need to grow tree depth? */ > > > - if (i < 0 && info->levels > ext2_dir_htree_level(fs)) > > > + if (i < 0 && info->levels >= ext2_dir_htree_level(fs)) > > > return EXT2_ET_DIR_NO_SPACE; > > > lblk = size / fs->blocksize; > > > size += fs->blocksize; > > > -- > > > 2.16.4 > > > > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-03-31 11:33 ` Lukas Czerner @ 2020-03-31 14:30 ` Jan Kara 2020-04-10 4:33 ` Theodore Y. Ts'o 0 siblings, 1 reply; 14+ messages in thread From: Jan Kara @ 2020-03-31 14:30 UTC (permalink / raw) To: Lukas Czerner; +Cc: Jan Kara, Ted Tso, linux-ext4 On Tue 31-03-20 13:33:03, Lukas Czerner wrote: > On Mon, Mar 30, 2020 at 04:55:31PM +0200, Jan Kara wrote: > > On Mon 30-03-20 15:27:12, Lukas Czerner wrote: > > > On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote: > > > > There is an off-by-one error in dx_grow_tree() when checking whether we > > > > can add another level to the tree. Thus we can grow tree too much > > > > leading to possible crashes in the library or corrupted filesystem. Fix > > > > the bug. > > > > > > Looks good, thanks > > > > > > Reviewed-by: Lukas Czerner <lczerner@redhat.com> > > > > > > Don't we have basically the same off-by-one in > > > e2fsck/pass1.c handle_htree() ? > > > > > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > > > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) > > > > I don't think so. It is indeed correct for root->indirect_levels to be > > equal to ext2_dir_htree_level(). However dx_grow_tree() is going to > > increase root->indirect_levels by 1 which is where tree would become > > invalid... > > > > Honza > > Hmm, are you sure ? I think the names are very confusing > > root->indirect_levels is zero based, while ext2_dir_htree_level() > returns the maximum number of levels (that is 3 by default). If I am > right then indirect_levels must always be smaller then > ext2_dir_htree_level() and that is how we use it everywhere else - the > palce I am pointing out is an exception and I think it's a bug. > > Indeed it looks like the bug got introduced in > 3f0cf647539970474be8f607017ca7eccfc2fbbe > > - if ((root->indirect_levels > 1) && > + if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > > Or am I missing something ? Ah, you're indeed right! e2fsck/pass2.c even has a correct version of the condition. Just the condition in pass1.c is wrong. Honza > > -Lukas > > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > --- > > > > lib/ext2fs/link.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c > > > > index 7b5bb022117c..469eea8cd06d 100644 > > > > --- a/lib/ext2fs/link.c > > > > +++ b/lib/ext2fs/link.c > > > > @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir, > > > > ext2fs_le16_to_cpu(info->frames[i].head->limit)) > > > > break; > > > > /* Need to grow tree depth? */ > > > > - if (i < 0 && info->levels > ext2_dir_htree_level(fs)) > > > > + if (i < 0 && info->levels >= ext2_dir_htree_level(fs)) > > > > return EXT2_ET_DIR_NO_SPACE; > > > > lblk = size / fs->blocksize; > > > > size += fs->blocksize; > > > > -- > > > > 2.16.4 > > > > > > > > > -- > > Jan Kara <jack@suse.com> > > SUSE Labs, CR > > > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-03-31 14:30 ` Jan Kara @ 2020-04-10 4:33 ` Theodore Y. Ts'o 2020-04-10 15:09 ` Jan Kara 0 siblings, 1 reply; 14+ messages in thread From: Theodore Y. Ts'o @ 2020-04-10 4:33 UTC (permalink / raw) To: Jan Kara; +Cc: Lukas Czerner, linux-ext4 On Tue, Mar 31, 2020 at 04:30:35PM +0200, Jan Kara wrote: > > > > Don't we have basically the same off-by-one in > > > > e2fsck/pass1.c handle_htree() ? > > > > > > > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > > > > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) > > > > > > > root->indirect_levels is zero based, while ext2_dir_htree_level() > > returns the maximum number of levels (that is 3 by default). If I am > > right then indirect_levels must always be smaller then > > ext2_dir_htree_level() and that is how we use it everywhere else - the > > palce I am pointing out is an exception and I think it's a bug. > > > > Indeed it looks like the bug got introduced in > > 3f0cf647539970474be8f607017ca7eccfc2fbbe > > > > - if ((root->indirect_levels > 1) && > > + if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > > > > Or am I missing something ? > > Ah, you're indeed right! e2fsck/pass2.c even has a correct version of the > condition. Just the condition in pass1.c is wrong. I've applied the following fix on the maint branch. - Ted commit 759b387775bfd5c9d3692680e5e4b929c3848d51 Author: Theodore Ts'o <tytso@mit.edu> Date: Fri Apr 10 00:30:52 2020 -0400 e2fsck: fix off-by-one check when validating depth of an htree Fixes: 3f0cf6475399 ("e2fsprogs: add support for 3-level htree") Signed-off-by: Theodore Ts'o <tytso@mit.edu> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c index c9e8bf82..38afda48 100644 --- a/e2fsck/pass1.c +++ b/e2fsck/pass1.c @@ -2685,7 +2685,7 @@ static int handle_htree(e2fsck_t ctx, struct problem_context *pctx, return 1; pctx->num = root->indirect_levels; - if ((root->indirect_levels > ext2_dir_htree_level(fs)) && + if ((root->indirect_levels >= ext2_dir_htree_level(fs)) && fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) return 1; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-04-10 4:33 ` Theodore Y. Ts'o @ 2020-04-10 15:09 ` Jan Kara 0 siblings, 0 replies; 14+ messages in thread From: Jan Kara @ 2020-04-10 15:09 UTC (permalink / raw) To: Theodore Y. Ts'o; +Cc: Jan Kara, Lukas Czerner, linux-ext4 On Fri 10-04-20 00:33:21, Theodore Y. Ts'o wrote: > On Tue, Mar 31, 2020 at 04:30:35PM +0200, Jan Kara wrote: > > > > > Don't we have basically the same off-by-one in > > > > > e2fsck/pass1.c handle_htree() ? > > > > > > > > > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > > > > > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) > > > > > > > > > > root->indirect_levels is zero based, while ext2_dir_htree_level() > > > returns the maximum number of levels (that is 3 by default). If I am > > > right then indirect_levels must always be smaller then > > > ext2_dir_htree_level() and that is how we use it everywhere else - the > > > palce I am pointing out is an exception and I think it's a bug. > > > > > > Indeed it looks like the bug got introduced in > > > 3f0cf647539970474be8f607017ca7eccfc2fbbe > > > > > > - if ((root->indirect_levels > 1) && > > > + if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > > > > > > Or am I missing something ? > > > > Ah, you're indeed right! e2fsck/pass2.c even has a correct version of the > > condition. Just the condition in pass1.c is wrong. > > I've applied the following fix on the maint branch. > > - Ted > > commit 759b387775bfd5c9d3692680e5e4b929c3848d51 > Author: Theodore Ts'o <tytso@mit.edu> > Date: Fri Apr 10 00:30:52 2020 -0400 > > e2fsck: fix off-by-one check when validating depth of an htree > > Fixes: 3f0cf6475399 ("e2fsprogs: add support for 3-level htree") > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> Cool, thanks! Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > > diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c > index c9e8bf82..38afda48 100644 > --- a/e2fsck/pass1.c > +++ b/e2fsck/pass1.c > @@ -2685,7 +2685,7 @@ static int handle_htree(e2fsck_t ctx, struct problem_context *pctx, > return 1; > > pctx->num = root->indirect_levels; > - if ((root->indirect_levels > ext2_dir_htree_level(fs)) && > + if ((root->indirect_levels >= ext2_dir_htree_level(fs)) && > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx)) > return 1; > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() 2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara 2020-03-30 13:27 ` Lukas Czerner @ 2020-04-10 4:32 ` Theodore Y. Ts'o 1 sibling, 0 replies; 14+ messages in thread From: Theodore Y. Ts'o @ 2020-04-10 4:32 UTC (permalink / raw) To: Jan Kara; +Cc: linux-ext4 On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote: > There is an off-by-one error in dx_grow_tree() when checking whether we > can add another level to the tree. Thus we can grow tree too much > leading to possible crashes in the library or corrupted filesystem. Fix > the bug. > > Signed-off-by: Jan Kara <jack@suse.cz> Applied, thanks. - Ted ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-04-10 15:09 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-30 9:09 [PATCH 0/2] Fixes for dir_index support in libext2fs Jan Kara 2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara 2020-03-30 13:24 ` Lukas Czerner 2020-03-30 13:48 ` Lukas Czerner 2020-03-30 14:55 ` Jan Kara 2020-04-10 3:52 ` Theodore Y. Ts'o 2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara 2020-03-30 13:27 ` Lukas Czerner 2020-03-30 14:55 ` Jan Kara 2020-03-31 11:33 ` Lukas Czerner 2020-03-31 14:30 ` Jan Kara 2020-04-10 4:33 ` Theodore Y. Ts'o 2020-04-10 15:09 ` Jan Kara 2020-04-10 4:32 ` Theodore Y. Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).