[PATCH] Fix minixfs size check

[PATCH] Fix minixfs size check

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

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 <phcoder@gmail.com>

diff --git a/fs/minix/itree_v1.c b/fs/minix/itree_v1.c
index 282e15a..4f8f8b2 100644
--- a/fs/minix/itree_v1.c
+++ b/fs/minix/itree_v1.c
@@ -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",
diff --git a/fs/minix/itree_v2.c b/fs/minix/itree_v2.c
index 13487ad..4a9a19d 100644
--- a/fs/minix/itree_v2.c
+++ b/fs/minix/itree_v2.c
@@ -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",


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] Fix minixfs size check

[Jan Kara]

On Sun 13-05-12 15:48:55, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> 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
> 
> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> 
> diff --git a/fs/minix/itree_v1.c b/fs/minix/itree_v1.c
> index 282e15a..4f8f8b2 100644
> --- a/fs/minix/itree_v1.c
> +++ b/fs/minix/itree_v1.c
> @@ -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",
> diff --git a/fs/minix/itree_v2.c b/fs/minix/itree_v2.c
> index 13487ad..4a9a19d 100644
> --- a/fs/minix/itree_v2.c
> +++ b/fs/minix/itree_v2.c
> @@ -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 <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] Fix minixfs size check

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

On 15.05.2012 00:19, Jan Kara wrote:

> On Sun 13-05-12 15:48:55, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> 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.
-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] Fix minixfs size check

[Jan Kara]

On Tue 15-05-12 00:33:52, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 15.05.2012 00:19, Jan Kara wrote:
> 
> > On Sun 13-05-12 15:48:55, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> 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.
  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

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH V2] Fix minixfs size check

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]


>   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

> 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 <phcoder@gmail.com>
---
 fs/minix/inode.c    |    1 +
 fs/minix/itree_v1.c |    3 ++-
 fs/minix/itree_v2.c |    3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index fcb05d2..133bb02 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -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.
 	 */
diff --git a/fs/minix/itree_v1.c b/fs/minix/itree_v1.c
index 282e15a..4f8f8b2 100644
--- a/fs/minix/itree_v1.c
+++ b/fs/minix/itree_v1.c
@@ -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",
diff --git a/fs/minix/itree_v2.c b/fs/minix/itree_v2.c
index 13487ad..4a9a19d 100644
--- a/fs/minix/itree_v2.c
+++ b/fs/minix/itree_v2.c
@@ -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",


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]