[PATCH] FS: nilfs2: clamps ns_r_segments_percentage to [1, 99]

[PATCH] FS: nilfs2: clamps ns_r_segments_percentage to [1, 99]

[Haogang Chen]

ns_r_segments_percentage is read from the disk. Bogus or malicious
value could cause integer overflow and potential security holes.
This patch reports error when mounting such bogus volumes.

Signed-off-by: Haogang Chen <haogangchen@gmail.com>
---
 fs/nilfs2/the_nilfs.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index d327140..e12b47b 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -409,6 +409,12 @@ static int nilfs_store_disk_layout(struct the_nilfs *nilfs,
 	nilfs->ns_first_data_block = le64_to_cpu(sbp->s_first_data_block);
 	nilfs->ns_r_segments_percentage =
 		le32_to_cpu(sbp->s_r_segments_percentage);
+	if (nilfs->ns_r_segments_percentage < 1 ||
+		nilfs->ns_r_segments_percentage > 99) {
+		printk(KERN_ERR "NILFS: invalid reserved segments percentage.\n");
+		return -EINVAL;
+	}
+
 	nilfs_set_nsegments(nilfs, le64_to_cpu(sbp->s_nsegments));
 	nilfs->ns_crc_seed = le32_to_cpu(sbp->s_crc_seed);
 	return 0;
-- 
1.7.5.4

Re: [PATCH] FS: nilfs2: clamps ns_r_segments_percentage to [1, 99]

[Ryusuke Konishi]

Hi, Chen

On Mon, 20 Feb 2012 16:55:30 -0500, Haogang Chen wrote:
> ns_r_segments_percentage is read from the disk. Bogus or malicious
> value could cause integer overflow and potential security holes.
> This patch reports error when mounting such bogus volumes.
> 
> Signed-off-by: Haogang Chen <haogangchen@gmail.com>

Ok, I will pick this up as a fix.

But this seems not to cause security issues; it just makes some disk
usage calculations meaningless and causes malfunction for such
out-of-range values.  Right?

May I amend the change log in terms of the impact ?


Thanks,
Ryusuke Konishi

> ---
>  fs/nilfs2/the_nilfs.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index d327140..e12b47b 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -409,6 +409,12 @@ static int nilfs_store_disk_layout(struct the_nilfs *nilfs,
>  	nilfs->ns_first_data_block = le64_to_cpu(sbp->s_first_data_block);
>  	nilfs->ns_r_segments_percentage =
>  		le32_to_cpu(sbp->s_r_segments_percentage);
> +	if (nilfs->ns_r_segments_percentage < 1 ||
> +		nilfs->ns_r_segments_percentage > 99) {
> +		printk(KERN_ERR "NILFS: invalid reserved segments percentage.\n");
> +		return -EINVAL;
> +	}
> +
>  	nilfs_set_nsegments(nilfs, le64_to_cpu(sbp->s_nsegments));
>  	nilfs->ns_crc_seed = le32_to_cpu(sbp->s_crc_seed);
>  	return 0;
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] FS: nilfs2: clamps ns_r_segments_percentage to [1, 99]

[Xi Wang]

On Feb 22, 2012, at 10:46 AM, Ryusuke Konishi wrote:
> But this seems not to cause security issues; it just makes some disk
> usage calculations meaningless and causes malfunction for such
> out-of-range values.  Right?

Seems true to me.

There may be another issue a few lines above: ns_blocks_per_segment
doesn't seem to have an upper bound (though it has a lower bound
NILFS_SEG_MIN_BLOCKS).  ns_blocks_per_segment is used in several
multiplications, such as in nilfs_ioctl_clean_segments:

if (argv[n].v_nmembs > nsegs * nilfs->ns_blocks_per_segment
        goto out_free;

Will this cause any problem?  Or is there any reasonable upper bound
for ns_blocks_per_segment?

- xi

Re: [PATCH] FS: nilfs2: clamps ns_r_segments_percentage to [1, 99]

[Ryusuke Konishi]

Hi,
On Wed, 22 Feb 2012 10:59:25 -0500, Xi Wang wrote:
> On Feb 22, 2012, at 10:46 AM, Ryusuke Konishi wrote:
> > But this seems not to cause security issues; it just makes some disk
> > usage calculations meaningless and causes malfunction for such
> > out-of-range values.  Right?
> 
> Seems true to me.
> 
> There may be another issue a few lines above: ns_blocks_per_segment
> doesn't seem to have an upper bound (though it has a lower bound
> NILFS_SEG_MIN_BLOCKS).  ns_blocks_per_segment is used in several
> multiplications, such as in nilfs_ioctl_clean_segments:
> 
> if (argv[n].v_nmembs > nsegs * nilfs->ns_blocks_per_segment
>         goto out_free;
> 
> Will this cause any problem?

Looks similar.  This may cause malfunction, but it seems not to cause
security issues.

However, this range check seems no longer make sense.. it looks
eliminable because the potential overflow issue was corrected by the
following fix:

                 if (argv[n].v_nmembs >= UINT_MAX / argv[n].v_size)
                        goto out_free;

Moreover, the upper bound "nsegs * nilfs->ns_blocks_per_segment" looks
improper for argv[1].v_nmembs.  The argument argv[1] is used to pass
an array of nilfs_period struct (ranges of deleting checkpoints).  But
the number of nilfs_period is not relate to the number of GCing
blocks.
 
> Or is there any reasonable upper bound
> for ns_blocks_per_segment?

Unclear.
It is not efficient to check overflow in each place.  So, if we can
define a minimum upper bound which can logically prevent overflow of
every related multiplication, we should add a pre range check with it.


Thanks,
Ryusuke Konishi
 
> - xi
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/