Patchwork [7/7] libext2fs: enforce the block group descriptor size in ext2fs_open()

login
register
mail settings
Submitter Theodore Ts'o
Date July 30, 2012, 9:47 p.m.
Message ID <1343684862-13181-7-git-send-email-tytso@mit.edu>
Download mbox | patch
Permalink /patch/174113/
State Accepted
Headers show

Comments

Theodore Ts'o - July 30, 2012, 9:47 p.m.
Since various parts of the library depend on the value of s_desc_size,
check to make sure it is the correct, expected value based on the file
system features.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/ext2_err.et.in |  3 +++
 lib/ext2fs/openfs.c       | 15 +++++++++++++++
 2 files changed, 18 insertions(+)
Andreas Dilger - July 31, 2012, 6:38 p.m.
On 2012-07-30, at 14:47, Theodore Ts'o <tytso@mit.edu> wrote:
> Since various parts of the library depend on the value of s_desc_size,
> check to make sure it is the correct, expected value based on the file
> system features.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
> lib/ext2fs/ext2_err.et.in |  3 +++
> lib/ext2fs/openfs.c       | 15 +++++++++++++++
> 2 files changed, 18 insertions(+)
> 
> diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
> index ccf1894..5987185 100644
> --- a/lib/ext2fs/ext2_err.et.in
> +++ b/lib/ext2fs/ext2_err.et.in
> @@ -443,4 +443,7 @@ ec    EXT2_ET_MMP_CHANGE_ABORT,
> ec    EXT2_ET_MMP_OPEN_DIRECT,
>    "MMP: open with O_DIRECT failed"
> 
> +ec    EXT2_ET_BAD_DESC_SIZE,
> +    "Block group descriptor size incorrect"
> +
>    end
> diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
> index 482e4ab..fbe9acd 100644
> --- a/lib/ext2fs/openfs.c
> +++ b/lib/ext2fs/openfs.c
> @@ -263,6 +263,21 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
>        retval = EXT2_ET_CORRUPT_SUPERBLOCK;
>        goto cleanup;
>    }
> +
> +    /* Enforce the block group descriptor size */
> +    if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
> +        if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {

It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. 

Cheers, Andreas

> +            retval = EXT2_ET_BAD_DESC_SIZE;
> +            goto cleanup;
> +        }
> +    } else {
> +        if (fs->super->s_desc_size &&
> +            fs->super->s_desc_size != EXT2_MIN_DESC_SIZE) {
> +            retval = EXT2_ET_BAD_DESC_SIZE;
> +            goto cleanup;
> +        }
> +    }
> +
>    fs->cluster_ratio_bits = fs->super->s_log_cluster_size -
>        fs->super->s_log_block_size;
>    if (EXT2_BLOCKS_PER_GROUP(fs->super) !=
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - July 31, 2012, 8:09 p.m.
On Tue, Jul 31, 2012 at 11:38:47AM -0700, Andreas Dilger wrote:
> > +    /* Enforce the block group descriptor size */
> > +    if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
> > +        if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {
> 
> It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. 

I'm not at all convinced that the ext2fs library will do the right
thing if the block group size is larger than what we expect.  At the
very least we need to make sure it is a power of two, but even so I'd
want to audit the code and do some experiments before I would hang my
hat on this actually working correctly...

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger - Aug. 1, 2012, 8:45 p.m.
On 2012-07-31, at 13:09, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Jul 31, 2012 at 11:38:47AM -0700, Andreas Dilger wrote:
>>> +    /* Enforce the block group descriptor size */
>>> +    if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
>>> +        if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {
>> 
>> It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. 
> 
> I'm not at all convinced that the ext2fs library will do the right
> thing if the block group size is larger than what we expect.  At the
> very least we need to make sure it is a power of two, but even so I'd
> want to audit the code and do some experiments before I would hang my
> hat on this actually working correctly...

I'm pretty sure that we've always checked that it is a power-of-two value. The problem with this change is that it makes it much harder to increase the size again in the future .

It would definitely need another feature flag, but it isn't clear without more investigation (that i can't do on my phone) if a COMPAT flag would be enough (which we would likely have anyway if we needed a larger descriptor) or if we need a more restrictive feature flag. 

Cheers, Andreas--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Aug. 3, 2012, midnight
On Wed, Aug 01, 2012 at 01:45:32PM -0700, Andreas Dilger wrote:
> 
> I'm pretty sure that we've always checked that it is a power-of-two
> value. The problem with this change is that it makes it much harder
> to increase the size again in the future .

Unfortunately, no, there aren't any such checks anywhere in either the
kernel or e2fsck.  In fact, e2fsck wasn't checking s_desc_size at all
(although it was depending on it), which is what caused me to get very
worried...

> It would definitely need another feature flag, but it isn't clear
> without more investigation (that i can't do on my phone) if a COMPAT
> flag would be enough (which we would likely have anyway if we needed
> a larger descriptor) or if we need a more restrictive feature flag.

It seems very likely that any new feature that would require us to
expand the block group descriptor would require other changes to the
file system's structures that would require a more restrictive
feature flag....

						- Ted


		
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
index ccf1894..5987185 100644
--- a/lib/ext2fs/ext2_err.et.in
+++ b/lib/ext2fs/ext2_err.et.in
@@ -443,4 +443,7 @@  ec	EXT2_ET_MMP_CHANGE_ABORT,
 ec	EXT2_ET_MMP_OPEN_DIRECT,
 	"MMP: open with O_DIRECT failed"
 
+ec	EXT2_ET_BAD_DESC_SIZE,
+	"Block group descriptor size incorrect"
+
 	end
diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
index 482e4ab..fbe9acd 100644
--- a/lib/ext2fs/openfs.c
+++ b/lib/ext2fs/openfs.c
@@ -263,6 +263,21 @@  errcode_t ext2fs_open2(const char *name, const char *io_options,
 		retval = EXT2_ET_CORRUPT_SUPERBLOCK;
 		goto cleanup;
 	}
+
+	/* Enforce the block group descriptor size */
+	if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
+		if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {
+			retval = EXT2_ET_BAD_DESC_SIZE;
+			goto cleanup;
+		}
+	} else {
+		if (fs->super->s_desc_size &&
+		    fs->super->s_desc_size != EXT2_MIN_DESC_SIZE) {
+			retval = EXT2_ET_BAD_DESC_SIZE;
+			goto cleanup;
+		}
+	}
+
 	fs->cluster_ratio_bits = fs->super->s_log_cluster_size -
 		fs->super->s_log_block_size;
 	if (EXT2_BLOCKS_PER_GROUP(fs->super) !=