Message ID | 20191023013112.18809-1-tytso@mit.edu |
---|---|
State | Rejected |
Headers | show |
Series | ext4: fix signed vs unsigned comparison in ext4_valid_extent() | expand |
On Tue, Oct 22, 2019 at 09:31:12PM -0400, Theodore Ts'o wrote: > Due to a signed vs unsigned comparison, an invalid extent where > ee_block (the logical block) is so large that lblk + len overflow > wasn't getting flagged as invalid. > > As a result, we tripped the BUG_ON(end < lblk) in > ext4_es_cache_extent() when trying to mount a file system with a > corrupted journal inode was corrupted. > > https://bugzilla.kernel.org/show_bug.cgi?id=205197 > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > Cc: stable@kernel.org > --- > fs/ext4/extents.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index fb0f99dc8c22..d12bc287abdc 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -367,7 +367,7 @@ ext4_ext_max_entries(struct inode *inode, int depth) > static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext) > { > ext4_fsblk_t block = ext4_ext_pblock(ext); > - int len = ext4_ext_get_actual_len(ext); > + unsigned int len = ext4_ext_get_actual_len(ext); > ext4_lblk_t lblock = le32_to_cpu(ext->ee_block); > > /* > -- > 2.23.0 > This patch can't be fixing anything because the comparison is unsigned both before and after this patch. - Eric
On Tue, Oct 22, 2019 at 10:44:47PM -0700, Eric Biggers wrote: > > This patch can't be fixing anything because the comparison is unsigned both > before and after this patch. Thanks, you're right; I had forgotten C's signed/unsigned rules for addition. The funny thing is the original reporter of BZ #205197 reported that the problem went away he tried a similar patch. - Ted
On Wed, Oct 23, 2019 at 09:15:46AM -0400, Theodore Y. Ts'o wrote: > On Tue, Oct 22, 2019 at 10:44:47PM -0700, Eric Biggers wrote: > > > > This patch can't be fixing anything because the comparison is unsigned both > > before and after this patch. > > Thanks, you're right; I had forgotten C's signed/unsigned rules for > addition. The funny thing is the original reporter of BZ #205197 > reported that the problem went away he tried a similar patch. Not trying to stick my nose in too much here but: What does it mean if ext4_ext_get_actual_len() to return < 0? Ira > > - Ted
On Wed, Oct 23, 2019 at 11:43:33AM -0700, Ira Weiny wrote: > On Wed, Oct 23, 2019 at 09:15:46AM -0400, Theodore Y. Ts'o wrote: > > On Tue, Oct 22, 2019 at 10:44:47PM -0700, Eric Biggers wrote: > > > > > > This patch can't be fixing anything because the comparison is unsigned both > > > before and after this patch. > > > > Thanks, you're right; I had forgotten C's signed/unsigned rules for > > addition. The funny thing is the original reporter of BZ #205197 > > reported that the problem went away he tried a similar patch. > > Not trying to stick my nose in too much here but: > > What does it mean if ext4_ext_get_actual_len() to return < 0? It's not possible for it to return < 0. We probably should clean it up to make it return an unsigned int, but that's a longer-term clean-up. - Ted
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index fb0f99dc8c22..d12bc287abdc 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -367,7 +367,7 @@ ext4_ext_max_entries(struct inode *inode, int depth) static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext) { ext4_fsblk_t block = ext4_ext_pblock(ext); - int len = ext4_ext_get_actual_len(ext); + unsigned int len = ext4_ext_get_actual_len(ext); ext4_lblk_t lblock = le32_to_cpu(ext->ee_block); /*
Due to a signed vs unsigned comparison, an invalid extent where ee_block (the logical block) is so large that lblk + len overflow wasn't getting flagged as invalid. As a result, we tripped the BUG_ON(end < lblk) in ext4_es_cache_extent() when trying to mount a file system with a corrupted journal inode was corrupted. https://bugzilla.kernel.org/show_bug.cgi?id=205197 Signed-off-by: Theodore Ts'o <tytso@mit.edu> Cc: stable@kernel.org --- fs/ext4/extents.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)