[PATCH 1/4] befs: check allocation_group number before use

[PATCH 1/4] befs: check allocation_group number before use

[Salah Triki]

Check that the allocation group number is not greater or equal to the
number of allocations group in the file system and return BEF_ERR in the
case of error.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 fs/befs/befs.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/befs/befs.h b/fs/befs/befs.h
index 55f3ea2..6daf4c4 100644
--- a/fs/befs/befs.h
+++ b/fs/befs/befs.h
@@ -121,6 +121,11 @@ BEFS_I(const struct inode *inode)
 static inline befs_blocknr_t
 iaddr2blockno(struct super_block *sb, const befs_inode_addr *iaddr)
 {
+	if (iaddr->allocation_group >= BEFS_SB(sb)->num_ags) {
+		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
+			   iaddr->allocation_group, BEFS_SB(sb)->num_ags);
+		return BEFS_ERR;
+	}
 	return ((iaddr->allocation_group << BEFS_SB(sb)->ag_shift) +
 		iaddr->start);
 }
-- 
1.9.1

[PATCH 2/4] befs: check return value of iaddr2blockno

[Salah Triki]

The call of iaddr2blockno may fail, so check its return value and
propagate error, if any.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 fs/befs/datastream.c | 30 ++++++++++++++++++++----------
 fs/befs/inode.c      | 10 ++++++++--
 fs/befs/io.c         |  3 +++
 fs/befs/linuxvfs.c   | 12 +++++++++++-
 4 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
index 343123c..1747f64 100644
--- a/fs/befs/datastream.c
+++ b/fs/befs/datastream.c
@@ -308,8 +308,11 @@ befs_find_brun_indirect(struct super_block *sb,
 	befs_disk_block_run *array;
 
 	befs_block_run indirect = data->indirect;
-	befs_blocknr_t indirblockno = iaddr2blockno(sb, &indirect);
 	int arraylen = befs_iaddrs_per_block(sb);
+	befs_blocknr_t indirblockno = iaddr2blockno(sb, &indirect);
+
+	if (indirblockno == BEFS_ERR)
+		return BEFS_ERR;
 
 	befs_debug(sb, "---> %s, find %lu", __func__, (unsigned long)blockno);
 
@@ -422,6 +425,7 @@ befs_find_brun_dblindirect(struct super_block *sb,
 	struct buffer_head *indir_block;
 	befs_block_run indir_run;
 	befs_disk_inode_addr *iaddr_array;
+	befs_blocknr_t block;
 
 	befs_blocknr_t indir_start_blk =
 	    data->max_indirect_range >> BEFS_SB(sb)->block_shift;
@@ -461,15 +465,17 @@ befs_find_brun_dblindirect(struct super_block *sb,
 		return BEFS_ERR;
 	}
 
-	dbl_indir_block =
-	    sb_bread(sb, iaddr2blockno(sb, &data->double_indirect) +
-					dbl_which_block);
+	block = iaddr2blockno(sb, &data->double_indirect);
+
+	if (block == BEFS_ERR)
+		return BEFS_ERR;
+
+	dbl_indir_block = sb_bread(sb, blockno + dbl_which_block);
+
 	if (dbl_indir_block == NULL) {
 		befs_error(sb, "%s couldn't read the "
 			   "double-indirect block at blockno %lu", __func__,
-			   (unsigned long)
-			   iaddr2blockno(sb, &data->double_indirect) +
-			   dbl_which_block);
+			   (unsigned long)blockno + dbl_which_block);
 		return BEFS_ERR;
 	}
 
@@ -488,12 +494,16 @@ befs_find_brun_dblindirect(struct super_block *sb,
 		return BEFS_ERR;
 	}
 
-	indir_block =
-	    sb_bread(sb, iaddr2blockno(sb, &indir_run) + which_block);
+	blockno = iaddr2blockno(sb, &indir_run);
+
+	if (blockno == BEFS_ERR)
+		return BEFS_ERR;
+
+	indir_block = sb_bread(sb, blockno + which_block);
 	if (indir_block == NULL) {
 		befs_error(sb, "%s couldn't read the indirect block "
 			   "at blockno %lu", __func__, (unsigned long)
-			   iaddr2blockno(sb, &indir_run) + which_block);
+			   blockno + which_block);
 		return BEFS_ERR;
 	}
 
diff --git a/fs/befs/inode.c b/fs/befs/inode.c
index fa4b718..495d270 100644
--- a/fs/befs/inode.c
+++ b/fs/befs/inode.c
@@ -21,6 +21,7 @@ befs_check_inode(struct super_block *sb, befs_inode * raw_inode,
 	u32 magic1 = fs32_to_cpu(sb, raw_inode->magic1);
 	befs_inode_addr ino_num = fsrun_to_cpu(sb, raw_inode->inode_num);
 	u32 flags = fs32_to_cpu(sb, raw_inode->flags);
+	befs_blocknr_t block;
 
 	/* check magic header. */
 	if (magic1 != BEFS_INODE_MAGIC1) {
@@ -33,10 +34,15 @@ befs_check_inode(struct super_block *sb, befs_inode * raw_inode,
 	/*
 	 * Sanity check2: inodes store their own block address. Check it.
 	 */
-	if (inode != iaddr2blockno(sb, &ino_num)) {
+	block = iaddr2blockno(sb, &ino_num);
+
+	if (block == BEFS_ERR)
+		return BEFS_ERR;
+
+	if (inode != block) {
 		befs_error(sb, "inode blocknr field disagrees with vfs "
 			   "VFS: %lu, Inode %lu", (unsigned long)
-			   inode, (unsigned long)iaddr2blockno(sb, &ino_num));
+			   inode, (unsigned long)block);
 		return BEFS_BAD_INODE;
 	}
 
diff --git a/fs/befs/io.c b/fs/befs/io.c
index 4223b77..14eef7d 100644
--- a/fs/befs/io.c
+++ b/fs/befs/io.c
@@ -42,6 +42,9 @@ befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
 
 	block = iaddr2blockno(sb, &iaddr);
 
+	if (block == BEFS_ERR)
+		goto error;
+
 	befs_debug(sb, "%s: offset = %lu", __func__, (unsigned long)block);
 
 	bh = sb_bread(sb, block);
diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index c57f831..6635515 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -143,6 +143,9 @@ befs_get_block(struct inode *inode, sector_t block,
 
 	disk_off = (ulong) iaddr2blockno(sb, &run);
 
+	if (disk_off == BEFS_ERR)
+		return -EINVAL;
+
 	map_bh(bh_result, inode->i_sb, disk_off);
 
 	befs_debug(sb, "<--- %s for inode %lu, block %ld, disk address %lu",
@@ -756,6 +759,7 @@ befs_fill_super(struct super_block *sb, void *data, int silent)
 	long ret = -EINVAL;
 	const unsigned long sb_block = 0;
 	const off_t x86_sb_off = 512;
+	befs_blocknr_t block;
 
 	save_mount_options(sb, data);
 
@@ -830,7 +834,13 @@ befs_fill_super(struct super_block *sb, void *data, int silent)
 	/* Set real blocksize of fs */
 	sb_set_blocksize(sb, (ulong) befs_sb->block_size);
 	sb->s_op = &befs_sops;
-	root = befs_iget(sb, iaddr2blockno(sb, &(befs_sb->root_dir)));
+
+	block = iaddr2blockno(sb, &(befs_sb->root_dir));
+
+	if (block == BEFS_ERR)
+		goto unacquire_priv_sbp;
+
+	root = befs_iget(sb, block);
 	if (IS_ERR(root)) {
 		ret = PTR_ERR(root);
 		goto unacquire_priv_sbp;
-- 
1.9.1

[PATCH 3/4] befs: remove the validation of allocation group number

[Salah Triki]

Validating the allocation group number is unnecessary since it will
be done by iaddr2blockno().

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 fs/befs/io.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/befs/io.c b/fs/befs/io.c
index 14eef7d..f9790f5 100644
--- a/fs/befs/io.c
+++ b/fs/befs/io.c
@@ -28,18 +28,11 @@ befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
 {
 	struct buffer_head *bh;
 	befs_blocknr_t block = 0;
-	struct befs_sb_info *befs_sb = BEFS_SB(sb);
 
 	befs_debug(sb, "---> Enter %s "
 		   "[%u, %hu, %hu]", __func__, iaddr.allocation_group,
 		   iaddr.start, iaddr.len);
 
-	if (iaddr.allocation_group > befs_sb->num_ags) {
-		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
-			   iaddr.allocation_group, befs_sb->num_ags);
-		goto error;
-	}
-
 	block = iaddr2blockno(sb, &iaddr);
 
 	if (block == BEFS_ERR)
-- 
1.9.1

[PATCH 4/4] befs: remove unnecessary initialization

[Salah Triki]

There is no need to init block, since it will be overwitten later by
iaddr2blockno().

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 fs/befs/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/befs/io.c b/fs/befs/io.c
index f9790f5..39d0dce 100644
--- a/fs/befs/io.c
+++ b/fs/befs/io.c
@@ -27,7 +27,7 @@ struct buffer_head *
 befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
 {
 	struct buffer_head *bh;
-	befs_blocknr_t block = 0;
+	befs_blocknr_t block;
 
 	befs_debug(sb, "---> Enter %s "
 		   "[%u, %hu, %hu]", __func__, iaddr.allocation_group,
-- 
1.9.1

Re: [PATCH 1/4] befs: check allocation_group number before use

[Luis de Bethencourt]

On 10/08/16 23:12, Salah Triki wrote:
> Check that the allocation group number is not greater or equal to the
> number of allocations group in the file system and return BEF_ERR in the
> case of error.
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  fs/befs/befs.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/befs/befs.h b/fs/befs/befs.h
> index 55f3ea2..6daf4c4 100644
> --- a/fs/befs/befs.h
> +++ b/fs/befs/befs.h
> @@ -121,6 +121,11 @@ BEFS_I(const struct inode *inode)
>  static inline befs_blocknr_t
>  iaddr2blockno(struct super_block *sb, const befs_inode_addr *iaddr)
>  {
> +	if (iaddr->allocation_group >= BEFS_SB(sb)->num_ags) {
> +		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
> +			   iaddr->allocation_group, BEFS_SB(sb)->num_ags);
> +		return BEFS_ERR;
> +	}
>  	return ((iaddr->allocation_group << BEFS_SB(sb)->ag_shift) +
>  		iaddr->start);
>  }
> 

Hi Salah,

To understand why would we check for this. When can this error happen? I mean,
when can iaddr2blockno() be called with an out of bounds allocation group?

Thanks,
Luis

Re: [PATCH 4/4] befs: remove unnecessary initialization

[Luis de Bethencourt]

On 10/08/16 23:12, Salah Triki wrote:
> There is no need to init block, since it will be overwitten later by
> iaddr2blockno().
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  fs/befs/io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/befs/io.c b/fs/befs/io.c
> index f9790f5..39d0dce 100644
> --- a/fs/befs/io.c
> +++ b/fs/befs/io.c
> @@ -27,7 +27,7 @@ struct buffer_head *
>  befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
>  {
>  	struct buffer_head *bh;
> -	befs_blocknr_t block = 0;
> +	befs_blocknr_t block;
>  
>  	befs_debug(sb, "---> Enter %s "
>  		   "[%u, %hu, %hu]", __func__, iaddr.allocation_group,
> 

Applied and merged to:
https://github.com/luisbg/linux-befs/commits/for-next

Thanks Salah :)
Luis

Re: [PATCH 1/4] befs: check allocation_group number before use

[Salah Triki]

On Thu, Aug 11, 2016 at 11:56:16AM +0100, Luis de Bethencourt wrote:
> On 10/08/16 23:12, Salah Triki wrote:
> > Check that the allocation group number is not greater or equal to the
> > number of allocations group in the file system and return BEF_ERR in the
> > case of error.
> > 
> > Signed-off-by: Salah Triki <salah.triki@gmail.com>
> > ---
> >  fs/befs/befs.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/fs/befs/befs.h b/fs/befs/befs.h
> > index 55f3ea2..6daf4c4 100644
> > --- a/fs/befs/befs.h
> > +++ b/fs/befs/befs.h
> > @@ -121,6 +121,11 @@ BEFS_I(const struct inode *inode)
> >  static inline befs_blocknr_t
> >  iaddr2blockno(struct super_block *sb, const befs_inode_addr *iaddr)
> >  {
> > +	if (iaddr->allocation_group >= BEFS_SB(sb)->num_ags) {
> > +		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
> > +			   iaddr->allocation_group, BEFS_SB(sb)->num_ags);
> > +		return BEFS_ERR;
> > +	}
> >  	return ((iaddr->allocation_group << BEFS_SB(sb)->ag_shift) +
> >  		iaddr->start);
> >  }
> > 
> 
> Hi Salah,
> 
> To understand why would we check for this. When can this error happen? I mean,
> when can iaddr2blockno() be called with an out of bounds allocation group?
> 
> Thanks,
> Luis

Hi Luis,

allocation group number is set to blockno >> BEFS_SB(sb)->ag_shift by 
blockno2iaddr(), so if ag_shift is not valid, an out of bound may occur. 
By now, thanx to your question ;), I think it's better to check the validity 
of ag_shift when the superblock is loaded.

Nack.

Thanx for the question :)
Salah 

Re: [PATCH 1/4] befs: check allocation_group number before use

[Luis de Bethencourt]

On 12/08/16 08:26, Salah Triki wrote:
> On Thu, Aug 11, 2016 at 11:56:16AM +0100, Luis de Bethencourt wrote:
>> On 10/08/16 23:12, Salah Triki wrote:
>>> Check that the allocation group number is not greater or equal to the
>>> number of allocations group in the file system and return BEF_ERR in the
>>> case of error.
>>>
>>> Signed-off-by: Salah Triki <salah.triki@gmail.com>
>>> ---
>>>  fs/befs/befs.h | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/fs/befs/befs.h b/fs/befs/befs.h
>>> index 55f3ea2..6daf4c4 100644
>>> --- a/fs/befs/befs.h
>>> +++ b/fs/befs/befs.h
>>> @@ -121,6 +121,11 @@ BEFS_I(const struct inode *inode)
>>>  static inline befs_blocknr_t
>>>  iaddr2blockno(struct super_block *sb, const befs_inode_addr *iaddr)
>>>  {
>>> +	if (iaddr->allocation_group >= BEFS_SB(sb)->num_ags) {
>>> +		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
>>> +			   iaddr->allocation_group, BEFS_SB(sb)->num_ags);
>>> +		return BEFS_ERR;
>>> +	}
>>>  	return ((iaddr->allocation_group << BEFS_SB(sb)->ag_shift) +
>>>  		iaddr->start);
>>>  }
>>>
>>
>> Hi Salah,
>>
>> To understand why would we check for this. When can this error happen? I mean,
>> when can iaddr2blockno() be called with an out of bounds allocation group?
>>
>> Thanks,
>> Luis
> 
> Hi Luis,
> 
> allocation group number is set to blockno >> BEFS_SB(sb)->ag_shift by 
> blockno2iaddr(), so if ag_shift is not valid, an out of bound may occur. 
> By now, thanx to your question ;), I think it's better to check the validity 
> of ag_shift when the superblock is loaded.
> 
> Nack.
> 
> Thanx for the question :)
> Salah 
> 

No problem :)

I will assume the Nack covers patch 1 to 3. Since they are all related.

Enjoy the weekend,
Luis

Re: [PATCH 1/4] befs: check allocation_group number before use

[Salah Triki]

On Fri, Aug 12, 2016 at 10:38:58AM +0100, Luis de Bethencourt wrote:
> On 12/08/16 08:26, Salah Triki wrote:
> > On Thu, Aug 11, 2016 at 11:56:16AM +0100, Luis de Bethencourt wrote:
> >> On 10/08/16 23:12, Salah Triki wrote:
> >>> Check that the allocation group number is not greater or equal to the
> >>> number of allocations group in the file system and return BEF_ERR in the
> >>> case of error.
> >>>
> >>> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> >>> ---
> >>>  fs/befs/befs.h | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/fs/befs/befs.h b/fs/befs/befs.h
> >>> index 55f3ea2..6daf4c4 100644
> >>> --- a/fs/befs/befs.h
> >>> +++ b/fs/befs/befs.h
> >>> @@ -121,6 +121,11 @@ BEFS_I(const struct inode *inode)
> >>>  static inline befs_blocknr_t
> >>>  iaddr2blockno(struct super_block *sb, const befs_inode_addr *iaddr)
> >>>  {
> >>> +	if (iaddr->allocation_group >= BEFS_SB(sb)->num_ags) {
> >>> +		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
> >>> +			   iaddr->allocation_group, BEFS_SB(sb)->num_ags);
> >>> +		return BEFS_ERR;
> >>> +	}
> >>>  	return ((iaddr->allocation_group << BEFS_SB(sb)->ag_shift) +
> >>>  		iaddr->start);
> >>>  }
> >>>
> >>
> >> Hi Salah,
> >>
> >> To understand why would we check for this. When can this error happen? I mean,
> >> when can iaddr2blockno() be called with an out of bounds allocation group?
> >>
> >> Thanks,
> >> Luis
> > 
> > Hi Luis,
> > 
> > allocation group number is set to blockno >> BEFS_SB(sb)->ag_shift by 
> > blockno2iaddr(), so if ag_shift is not valid, an out of bound may occur. 
> > By now, thanx to your question ;), I think it's better to check the validity 
> > of ag_shift when the superblock is loaded.
> > 
> > Nack.
> > 
> > Thanx for the question :)
> > Salah 
> > 
> 
> No problem :)
> 
> I will assume the Nack covers patch 1 to 3. Since they are all related.
> 
> Enjoy the weekend,
> Luis

You too.
Salah