Fix minixfs size check

PATCH Superseded Vladimir 'φ-coder/phcoder' Serbinenko 2 participants 5 messages May 13, 2012 – May 14, 2012 VFS

lore

Share & tools

lore

Vladimir 'φ-coder/phcoder' Serbinenko

author

2012-05-13 13:48 UTC #0 lore raw

PATCH 1/1 Fix minixfs size check

minixfs file size check is buggy and it doesn't allow creating a block which can't be fully filled

Signed-off-by Vladimir Serbinenko <[email protected]>

fs/minix/itree_v1.c +2 -1

index 282e15a..4f8f8b2 100644
@@ -29,7 +29,8 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 	if (block < 0) {
 		printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
 			block, bdevname(inode->i_sb->s_bdev, b));
-	} else if (block >= (minix_sb(inode->i_sb)->s_max_size/BLOCK_SIZE)) {
+	} else if ((u64) block * (u64) BLOCK_SIZE
+		   >= minix_sb(inode->i_sb)->s_max_size) {
 		if (printk_ratelimit())
 			printk("MINIX-fs: block_to_path: "
 			       "block %ld too big on dev %s\n",

fs/minix/itree_v2.c +2 -1

index 13487ad..4a9a19d 100644
@@ -32,7 +32,8 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 	if (block < 0) {
 		printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
 			block, bdevname(sb->s_bdev, b));
-	} else if (block >= (minix_sb(inode->i_sb)->s_max_size/sb->s_blocksize)) {
+	} else if ((u64) block * (u64) sb->s_blocksize
+		   >= minix_sb(inode->i_sb)->s_max_size) {
 		if (printk_ratelimit())
 			printk("MINIX-fs: block_to_path: "
 			       "block %ld too big on dev %s\n",

Jan Kara

reviewer

2012-05-14 22:19 UTC #1 lore raw

Quoted 2 lines · Vladimir 'φ-coder/phcoder' Serbinenko

minixfs file size check is buggy and it doesn't allow creating a block which can't be fully filled

Umm, I'm not really minix expert but who'd set s_max_size to something
which is not a multiple of block size? This looks rather artifical problem to me...

Honza

Quoted 32 lines

Signed-off-by Vladimir Serbinenko <[email protected]>

fs/minix/itree_v1.c +2 -1

index 282e15a..4f8f8b2 100644
@@ -29,7 +29,8 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 	if (block < 0) {
 		printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
 			block, bdevname(inode->i_sb->s_bdev, b));
-	} else if (block >= (minix_sb(inode->i_sb)->s_max_size/BLOCK_SIZE)) {
+	} else if ((u64) block * (u64) BLOCK_SIZE
+		   >= minix_sb(inode->i_sb)->s_max_size) {
 		if (printk_ratelimit())
 			printk("MINIX-fs: block_to_path: "
 			       "block %ld too big on dev %s\n",

fs/minix/itree_v2.c +2 -1

index 13487ad..4a9a19d 100644
@@ -32,7 +32,8 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 	if (block < 0) {
 		printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
 			block, bdevname(sb->s_bdev, b));
-	} else if (block >= (minix_sb(inode->i_sb)->s_max_size/sb->s_blocksize)) {
+	} else if ((u64) block * (u64) sb->s_blocksize
+		   >= minix_sb(inode->i_sb)->s_max_size) {
 		if (printk_ratelimit())
 			printk("MINIX-fs: block_to_path: "
 			       "block %ld too big on dev %s\n",

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Vladimir 'φ-coder/phcoder' Serbinenko

author

2012-05-14 22:33 UTC #2 lore raw

Quoted 7 lines · Jan Kara

Quoted 2 lines · Vladimir 'φ-coder/phcoder' Serbinenko
minixfs file size check is buggy and it doesn't allow creating a block which can't be fully filled
Umm, I'm not really minix expert but who'd set s_max_size to something
which is not a multiple of block size? This looks rather artifical problem to me...

The usual and natural limit comes from interpreting 32-bit size field as signed or unsigned. So it's either 2G - 1 or 4G - 1. Neither of which is a multiple of block size.

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko

Jan Kara

reviewer

2012-05-14 23:02 UTC #3 lore raw

Quoted 5 lines · Vladimir 'φ-coder/phcoder' Serbinenko

Quoted 3 lines · Jan Kara
Quoted 2 lines · Vladimir 'φ-coder/phcoder' Serbinenko
minixfs file size check is buggy and it doesn't allow creating a block which can't be fully filled

Quoted 8 lines

Quoted 4 lines
Umm, I'm not really minix expert but who'd set s_max_size to something
which is not a multiple of block size? This looks rather artifical problem to me...
The usual and natural limit comes from interpreting 32-bit size field as signed or unsigned. So it's either 2G - 1 or 4G - 1. Neither of which is a multiple of block size.

Oh, right. Then your patch should be OK, just it's enough to cast one of
the arguments to u64. And BTW looking at minix, it should also set s_maxbytes to s_max_size. Otherwise it will be always limited by MAX_NON_LFS which is 2^31-1.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Vladimir 'φ-coder/phcoder' Serbinenko

author

2012-05-14 23:38 UTC #4 lore raw

PATCH v2 1/1 Fix minixfs size check

Quoted 3 lines

Oh, right. Then your patch should be OK, just it's enough to cast one of
the arguments to u64.

I know. I just consider it clearer and less risk to lose it in the future. Moreover it avoids thinking of how much is really needed

Quoted 3 lines

And BTW looking at minix, it should also set s_maxbytes to s_max_size. Otherwise it will be always limited by MAX_NON_LFS which is 2^31-1.

Patch here:

minixfs file size check is buggy and it doesn't allow creating a block which can't be fully filled

Signed-off-by Vladimir Serbinenko <[email protected]>

---
 fs/minix/inode.c    |    1 +
 fs/minix/itree_v1.c |    3 ++-
 fs/minix/itree_v2.c |    3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

fs/minix/inode.c +1 -0

index fcb05d2..133bb02 100644
@@ -227,6 +227,7 @@ static int minix_fill_super(struct super_block *s, void *data, int silent)
 	} else
 		goto out_no_fs;
 
+	s->s_maxbytes = sbi->s_max_size;
 	/*
 	 * Allocate the buffer map to keep the superblock small.
 	 */

fs/minix/itree_v1.c +2 -1

index 282e15a..4f8f8b2 100644
@@ -29,7 +29,8 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 	if (block < 0) {
 		printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
 			block, bdevname(inode->i_sb->s_bdev, b));
-	} else if (block >= (minix_sb(inode->i_sb)->s_max_size/BLOCK_SIZE)) {
+	} else if ((u64) block * (u64) BLOCK_SIZE
+		   >= minix_sb(inode->i_sb)->s_max_size) {
 		if (printk_ratelimit())
 			printk("MINIX-fs: block_to_path: "
 			       "block %ld too big on dev %s\n",

fs/minix/itree_v2.c +2 -1

index 13487ad..4a9a19d 100644
@@ -32,7 +32,8 @@ static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
 	if (block < 0) {
 		printk("MINIX-fs: block_to_path: block %ld < 0 on dev %s\n",
 			block, bdevname(sb->s_bdev, b));
-	} else if (block >= (minix_sb(inode->i_sb)->s_max_size/sb->s_blocksize)) {
+	} else if ((u64) block * (u64) sb->s_blocksize
+		   >= minix_sb(inode->i_sb)->s_max_size) {
 		if (printk_ratelimit())
 			printk("MINIX-fs: block_to_path: "
 			       "block %ld too big on dev %s\n",

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko