All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ext4: fix bitmap position validation
@ 2018-04-24 10:56 Lukas Czerner
  2018-04-24 15:43 ` Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: Lukas Czerner @ 2018-04-24 10:56 UTC (permalink / raw)
  To: linux-ext4; +Cc: Lukas Czerner, stable

Currently in ext4_valid_block_bitmap() we expect the bitmap to be
positioned anywhere between 0 and s_blocksize clusters, but that's
wrong because the bitmap can be placed anywhere in the block group. This
causes false positives when validating bitmaps on perfectly valid file
system layouts. Fix it by checking whether the bitmap is within the group
boundary.

The problem can be reproduced using the following

mkfs -t ext3 -E stride=256 /dev/vdb1
mount /dev/vdb1 /mnt/test
cd /mnt/test
wget https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.16.3.tar.xz
tar xf linux-4.16.3.tar.xz

This will result in the warnings in the logs

EXT4-fs error (device vdb1): ext4_validate_block_bitmap:399: comm tar: bg 84: block 2774529: invalid block bitmap

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Ilya Dryomov <idryomov@gmail.com>
Fixes: 7dac4a1726a9 ("ext4: add validity checks for bitmap block numbers")
Cc: stable@vger.kernel.org
---
 fs/ext4/balloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index a33d8fb..0c0e383 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -338,7 +338,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
 	/* check whether block bitmap block number is set */
 	blk = ext4_block_bitmap(sb, desc);
 	offset = blk - group_first_block;
-	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
+	if (offset < 0 ||
+	    EXT4_B2C(sbi, offset) >= EXT4_CLUSTERS_PER_GROUP(sb) ||
 	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
 		/* bad block bitmap */
 		return blk;
@@ -346,7 +347,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
 	/* check whether the inode bitmap block number is set */
 	blk = ext4_inode_bitmap(sb, desc);
 	offset = blk - group_first_block;
-	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
+	if (offset < 0 ||
+	    EXT4_B2C(sbi, offset) >= EXT4_CLUSTERS_PER_GROUP(sb) ||
 	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
 		/* bad block bitmap */
 		return blk;
@@ -354,8 +356,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
 	/* check whether the inode table block number is set */
 	blk = ext4_inode_table(sb, desc);
 	offset = blk - group_first_block;
-	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
-	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize)
+	if (offset < 0 || EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >=
+				   EXT4_CLUSTERS_PER_GROUP(sb))
 		return blk;
 	next_zero_bit = ext4_find_next_zero_bit(bh->b_data,
 			EXT4_B2C(sbi, offset + sbi->s_itb_per_group),
-- 
2.7.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] ext4: fix bitmap position validation
  2018-04-24 10:56 [PATCH] ext4: fix bitmap position validation Lukas Czerner
@ 2018-04-24 15:43 ` Theodore Y. Ts'o
  2018-04-25  7:39   ` Lukas Czerner
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Y. Ts'o @ 2018-04-24 15:43 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: linux-ext4, stable

On Tue, Apr 24, 2018 at 12:56:54PM +0200, Lukas Czerner wrote:
> @@ -354,8 +356,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
>  	/* check whether the inode table block number is set */
>  	blk = ext4_inode_table(sb, desc);
>  	offset = blk - group_first_block;
> -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> -	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize)
> +	if (offset < 0 || EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >=
> +				   EXT4_CLUSTERS_PER_GROUP(sb))
>  		return blk;
>  	next_zero_bit = ext4_find_next_zero_bit(bh->b_data,
>  			EXT4_B2C(sbi, offset + sbi->s_itb_per_group),

The two checks of offset and offset + sbi->s_itb_per_group are
necessary because a maliciously crafted file system can take advantage
of unsigned integer overflow such that offset is a very large number,
but offset + sbi->s_itb_per_group is a legal offset.  So we have ot
keep both checks.  As it happens I was working on a similar patch (but
was slowed down by my attendance at LSF/Mm).  So I've combined your
patch with mine, and came up with this.

					- Ted

>From 33444e3f7da8ae9840286732c0d7bbf8f9d8471b Mon Sep 17 00:00:00 2001
From: Lukas Czerner <lczerner@redhat.com>
Date: Tue, 24 Apr 2018 11:31:44 -0400
Subject: [PATCH] ext4: fix bitmap position validation

Currently in ext4_valid_block_bitmap() we expect the bitmap to be
positioned anywhere between 0 and s_blocksize clusters, but that's
wrong because the bitmap can be placed anywhere in the block group. This
causes false positives when validating bitmaps on perfectly valid file
system layouts. Fix it by checking whether the bitmap is within the group
boundary.

The problem can be reproduced using the following

mkfs -t ext3 -E stride=256 /dev/vdb1
mount /dev/vdb1 /mnt/test
cd /mnt/test
wget https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.16.3.tar.xz
tar xf linux-4.16.3.tar.xz

This will result in the warnings in the logs

EXT4-fs error (device vdb1): ext4_validate_block_bitmap:399: comm tar: bg 84: block 2774529: invalid block bitmap

[ Changed slightly for clarity and to not drop a overflow test -- TYT ]

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Reported-by: Ilya Dryomov <idryomov@gmail.com>
Fixes: 7dac4a1726a9 ("ext4: add validity checks for bitmap block numbers")
Cc: stable@vger.kernel.org
---
 fs/ext4/balloc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index a33d8fb1bf2a..508b905d744d 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -321,6 +321,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	ext4_grpblk_t offset;
 	ext4_grpblk_t next_zero_bit;
+	ext4_grpblk_t max_bit = EXT4_CLUSTERS_PER_GROUP(sb);
 	ext4_fsblk_t blk;
 	ext4_fsblk_t group_first_block;
 
@@ -338,7 +339,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
 	/* check whether block bitmap block number is set */
 	blk = ext4_block_bitmap(sb, desc);
 	offset = blk - group_first_block;
-	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
+	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
 	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
 		/* bad block bitmap */
 		return blk;
@@ -346,7 +347,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
 	/* check whether the inode bitmap block number is set */
 	blk = ext4_inode_bitmap(sb, desc);
 	offset = blk - group_first_block;
-	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
+	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
 	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
 		/* bad block bitmap */
 		return blk;
@@ -354,8 +355,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
 	/* check whether the inode table block number is set */
 	blk = ext4_inode_table(sb, desc);
 	offset = blk - group_first_block;
-	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
-	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize)
+	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
+	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= max_bit)
 		return blk;
 	next_zero_bit = ext4_find_next_zero_bit(bh->b_data,
 			EXT4_B2C(sbi, offset + sbi->s_itb_per_group),
-- 
2.16.1.72.g5be1f00a9a

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] ext4: fix bitmap position validation
  2018-04-24 15:43 ` Theodore Y. Ts'o
@ 2018-04-25  7:39   ` Lukas Czerner
  2018-04-30  8:24     ` Lukas Czerner
  0 siblings, 1 reply; 6+ messages in thread
From: Lukas Czerner @ 2018-04-25  7:39 UTC (permalink / raw)
  To: Theodore Y. Ts'o; +Cc: linux-ext4, stable

On Tue, Apr 24, 2018 at 11:43:30AM -0400, Theodore Y. Ts'o wrote:
> On Tue, Apr 24, 2018 at 12:56:54PM +0200, Lukas Czerner wrote:
> > @@ -354,8 +356,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
> >  	/* check whether the inode table block number is set */
> >  	blk = ext4_inode_table(sb, desc);
> >  	offset = blk - group_first_block;
> > -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> > -	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize)
> > +	if (offset < 0 || EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >=
> > +				   EXT4_CLUSTERS_PER_GROUP(sb))
> >  		return blk;
> >  	next_zero_bit = ext4_find_next_zero_bit(bh->b_data,
> >  			EXT4_B2C(sbi, offset + sbi->s_itb_per_group),
> 
> The two checks of offset and offset + sbi->s_itb_per_group are
> necessary because a maliciously crafted file system can take advantage
> of unsigned integer overflow such that offset is a very large number,
> but offset + sbi->s_itb_per_group is a legal offset.  So we have ot
> keep both checks.  As it happens I was working on a similar patch (but
> was slowed down by my attendance at LSF/Mm).  So I've combined your
> patch with mine, and came up with this.

Hi Ted,

maybe I am missing something but offset is signed int, but
s_itb_per_group is unsigned long, so if I recall the arithmetic
conversions correctly the offset will be converted to unsigned long and
the restult will be unsigned long.
Moreover s_itb_per_group value is sanitized when read and can't be very
big so the restult will always fit into unsigned long hence no overflow
is possible. Anyway that was my thought process when I removed the
additional check.

However it we have a maliciously created fs then

blk = ext4_inode_table(sb, desc);

might be either too big or too small and so

offset = blk - group_first_block;

might over/underflow giving us wrong offset that still by chance can land
into the block group. So maybe we need to check that blk is within the file
system and/or that offset does not overflow. Maybe making offset type
ext4_fsblk_t and checking that the result satisfies (offset <= blk) will
be enough ?

Have fun at LSF/MM :)

-Lukas

> 
> 					- Ted
> 
> From 33444e3f7da8ae9840286732c0d7bbf8f9d8471b Mon Sep 17 00:00:00 2001
> From: Lukas Czerner <lczerner@redhat.com>
> Date: Tue, 24 Apr 2018 11:31:44 -0400
> Subject: [PATCH] ext4: fix bitmap position validation
> 
> Currently in ext4_valid_block_bitmap() we expect the bitmap to be
> positioned anywhere between 0 and s_blocksize clusters, but that's
> wrong because the bitmap can be placed anywhere in the block group. This
> causes false positives when validating bitmaps on perfectly valid file
> system layouts. Fix it by checking whether the bitmap is within the group
> boundary.
> 
> The problem can be reproduced using the following
> 
> mkfs -t ext3 -E stride=256 /dev/vdb1
> mount /dev/vdb1 /mnt/test
> cd /mnt/test
> wget https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.16.3.tar.xz
> tar xf linux-4.16.3.tar.xz
> 
> This will result in the warnings in the logs
> 
> EXT4-fs error (device vdb1): ext4_validate_block_bitmap:399: comm tar: bg 84: block 2774529: invalid block bitmap
> 
> [ Changed slightly for clarity and to not drop a overflow test -- TYT ]
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Reported-by: Ilya Dryomov <idryomov@gmail.com>
> Fixes: 7dac4a1726a9 ("ext4: add validity checks for bitmap block numbers")
> Cc: stable@vger.kernel.org
> ---
>  fs/ext4/balloc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index a33d8fb1bf2a..508b905d744d 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -321,6 +321,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	ext4_grpblk_t offset;
>  	ext4_grpblk_t next_zero_bit;
> +	ext4_grpblk_t max_bit = EXT4_CLUSTERS_PER_GROUP(sb);
>  	ext4_fsblk_t blk;
>  	ext4_fsblk_t group_first_block;
>  
> @@ -338,7 +339,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
>  	/* check whether block bitmap block number is set */
>  	blk = ext4_block_bitmap(sb, desc);
>  	offset = blk - group_first_block;
> -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> +	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
>  	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
>  		/* bad block bitmap */
>  		return blk;
> @@ -346,7 +347,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
>  	/* check whether the inode bitmap block number is set */
>  	blk = ext4_inode_bitmap(sb, desc);
>  	offset = blk - group_first_block;
> -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> +	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
>  	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
>  		/* bad block bitmap */
>  		return blk;
> @@ -354,8 +355,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
>  	/* check whether the inode table block number is set */
>  	blk = ext4_inode_table(sb, desc);
>  	offset = blk - group_first_block;
> -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> -	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize)
> +	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
> +	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= max_bit)
>  		return blk;
>  	next_zero_bit = ext4_find_next_zero_bit(bh->b_data,
>  			EXT4_B2C(sbi, offset + sbi->s_itb_per_group),
> -- 
> 2.16.1.72.g5be1f00a9a
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ext4: fix bitmap position validation
  2018-04-25  7:39   ` Lukas Czerner
@ 2018-04-30  8:24     ` Lukas Czerner
  2018-04-30 16:45       ` Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: Lukas Czerner @ 2018-04-30  8:24 UTC (permalink / raw)
  To: Theodore Y. Ts'o; +Cc: linux-ext4, stable

On Wed, Apr 25, 2018 at 09:39:29AM +0200, Lukas Czerner wrote:
> On Tue, Apr 24, 2018 at 11:43:30AM -0400, Theodore Y. Ts'o wrote:
> > On Tue, Apr 24, 2018 at 12:56:54PM +0200, Lukas Czerner wrote:
> > > @@ -354,8 +356,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
> > >  	/* check whether the inode table block number is set */
> > >  	blk = ext4_inode_table(sb, desc);
> > >  	offset = blk - group_first_block;
> > > -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> > > -	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize)
> > > +	if (offset < 0 || EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >=
> > > +				   EXT4_CLUSTERS_PER_GROUP(sb))
> > >  		return blk;
> > >  	next_zero_bit = ext4_find_next_zero_bit(bh->b_data,
> > >  			EXT4_B2C(sbi, offset + sbi->s_itb_per_group),
> > 
> > The two checks of offset and offset + sbi->s_itb_per_group are
> > necessary because a maliciously crafted file system can take advantage
> > of unsigned integer overflow such that offset is a very large number,
> > but offset + sbi->s_itb_per_group is a legal offset.  So we have ot
> > keep both checks.  As it happens I was working on a similar patch (but
> > was slowed down by my attendance at LSF/Mm).  So I've combined your
> > patch with mine, and came up with this.
> 
> Hi Ted,
> 
> maybe I am missing something but offset is signed int, but
> s_itb_per_group is unsigned long, so if I recall the arithmetic
> conversions correctly the offset will be converted to unsigned long and
> the restult will be unsigned long.
> Moreover s_itb_per_group value is sanitized when read and can't be very
> big so the restult will always fit into unsigned long hence no overflow
> is possible. Anyway that was my thought process when I removed the
> additional check.
> 
> However it we have a maliciously created fs then
> 
> blk = ext4_inode_table(sb, desc);
> 
> might be either too big or too small and so
> 
> offset = blk - group_first_block;
> 
> might over/underflow giving us wrong offset that still by chance can land
> into the block group. So maybe we need to check that blk is within the file
> system and/or that offset does not overflow. Maybe making offset type
> ext4_fsblk_t and checking that the result satisfies (offset <= blk) will
> be enough ?
> 
> Have fun at LSF/MM :)
> 
> -Lukas

Well, so much for a discussion :-/ The patch is upstream now...

> 
> > 
> > 					- Ted
> > 
> > From 33444e3f7da8ae9840286732c0d7bbf8f9d8471b Mon Sep 17 00:00:00 2001
> > From: Lukas Czerner <lczerner@redhat.com>
> > Date: Tue, 24 Apr 2018 11:31:44 -0400
> > Subject: [PATCH] ext4: fix bitmap position validation
> > 
> > Currently in ext4_valid_block_bitmap() we expect the bitmap to be
> > positioned anywhere between 0 and s_blocksize clusters, but that's
> > wrong because the bitmap can be placed anywhere in the block group. This
> > causes false positives when validating bitmaps on perfectly valid file
> > system layouts. Fix it by checking whether the bitmap is within the group
> > boundary.
> > 
> > The problem can be reproduced using the following
> > 
> > mkfs -t ext3 -E stride=256 /dev/vdb1
> > mount /dev/vdb1 /mnt/test
> > cd /mnt/test
> > wget https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.16.3.tar.xz
> > tar xf linux-4.16.3.tar.xz
> > 
> > This will result in the warnings in the logs
> > 
> > EXT4-fs error (device vdb1): ext4_validate_block_bitmap:399: comm tar: bg 84: block 2774529: invalid block bitmap
> > 
> > [ Changed slightly for clarity and to not drop a overflow test -- TYT ]
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Reported-by: Ilya Dryomov <idryomov@gmail.com>
> > Fixes: 7dac4a1726a9 ("ext4: add validity checks for bitmap block numbers")
> > Cc: stable@vger.kernel.org
> > ---
> >  fs/ext4/balloc.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> > index a33d8fb1bf2a..508b905d744d 100644
> > --- a/fs/ext4/balloc.c
> > +++ b/fs/ext4/balloc.c
> > @@ -321,6 +321,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
> >  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> >  	ext4_grpblk_t offset;
> >  	ext4_grpblk_t next_zero_bit;
> > +	ext4_grpblk_t max_bit = EXT4_CLUSTERS_PER_GROUP(sb);
> >  	ext4_fsblk_t blk;
> >  	ext4_fsblk_t group_first_block;
> >  
> > @@ -338,7 +339,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
> >  	/* check whether block bitmap block number is set */
> >  	blk = ext4_block_bitmap(sb, desc);
> >  	offset = blk - group_first_block;
> > -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> > +	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
> >  	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
> >  		/* bad block bitmap */
> >  		return blk;
> > @@ -346,7 +347,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
> >  	/* check whether the inode bitmap block number is set */
> >  	blk = ext4_inode_bitmap(sb, desc);
> >  	offset = blk - group_first_block;
> > -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> > +	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
> >  	    !ext4_test_bit(EXT4_B2C(sbi, offset), bh->b_data))
> >  		/* bad block bitmap */
> >  		return blk;
> > @@ -354,8 +355,8 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct super_block *sb,
> >  	/* check whether the inode table block number is set */
> >  	blk = ext4_inode_table(sb, desc);
> >  	offset = blk - group_first_block;
> > -	if (offset < 0 || EXT4_B2C(sbi, offset) >= sb->s_blocksize ||
> > -	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= sb->s_blocksize)
> > +	if (offset < 0 || EXT4_B2C(sbi, offset) >= max_bit ||
> > +	    EXT4_B2C(sbi, offset + sbi->s_itb_per_group) >= max_bit)
> >  		return blk;
> >  	next_zero_bit = ext4_find_next_zero_bit(bh->b_data,
> >  			EXT4_B2C(sbi, offset + sbi->s_itb_per_group),
> > -- 
> > 2.16.1.72.g5be1f00a9a
> > 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ext4: fix bitmap position validation
  2018-04-30  8:24     ` Lukas Czerner
@ 2018-04-30 16:45       ` Theodore Y. Ts'o
  2018-05-02 11:44         ` Lukas Czerner
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Y. Ts'o @ 2018-04-30 16:45 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: linux-ext4, stable

On Mon, Apr 30, 2018 at 10:24:53AM +0200, Lukas Czerner wrote:
> 
> Well, so much for a discussion :-/ The patch is upstream now...

Sorry, it's been crazy busy and there were other fixes I really needed
to get upstream.

If it really is superfluous, the compiler will optimize it away.  I
have been bitten more times than I can count with fuzz testers and C's
_fine_ signed vs. unsigned integer semantics.  So I prefer to be
careful here.

If we can really be sure it's safe, we can always remove the extra
comparison in a separate patch, since that's unrelated to the real bug
we're trying to fix.

						- Ted

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ext4: fix bitmap position validation
  2018-04-30 16:45       ` Theodore Y. Ts'o
@ 2018-05-02 11:44         ` Lukas Czerner
  0 siblings, 0 replies; 6+ messages in thread
From: Lukas Czerner @ 2018-05-02 11:44 UTC (permalink / raw)
  To: Theodore Y. Ts'o; +Cc: linux-ext4, stable

On Mon, Apr 30, 2018 at 12:45:07PM -0400, Theodore Y. Ts'o wrote:
> On Mon, Apr 30, 2018 at 10:24:53AM +0200, Lukas Czerner wrote:
> > 
> > Well, so much for a discussion :-/ The patch is upstream now...
> 
> Sorry, it's been crazy busy and there were other fixes I really needed
> to get upstream.

Yeah, I understand.

> 
> If it really is superfluous, the compiler will optimize it away.  I
> have been bitten more times than I can count with fuzz testers and C's
> _fine_ signed vs. unsigned integer semantics.  So I prefer to be
> careful here.
> 
> If we can really be sure it's safe, we can always remove the extra
> comparison in a separate patch, since that's unrelated to the real bug
> we're trying to fix.

That's fine, I understand that it was better to fix it sooner rather
than later and deal with the optimization at a different time. Short
reply stating that would have helped though, there is my sign-off below
the patch as well after all.

-Lukas

> 
> 						- Ted

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-02 11:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-24 10:56 [PATCH] ext4: fix bitmap position validation Lukas Czerner
2018-04-24 15:43 ` Theodore Y. Ts'o
2018-04-25  7:39   ` Lukas Czerner
2018-04-30  8:24     ` Lukas Czerner
2018-04-30 16:45       ` Theodore Y. Ts'o
2018-05-02 11:44         ` Lukas Czerner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.