Re: [PATCH 1/7] ext4: avoid overflow when setting values via sysfs

[Jan Kara <jack@suse.cz>]

On Sat 17-02-24 15:09:06, Baokun Li wrote:
> On 2024/2/14 0:05, Jan Kara wrote:
> > On Fri 26-01-24 16:57:10, Baokun Li wrote:
> > > When setting values of type unsigned int through sysfs, we use kstrtoul()
> > > to parse it and then truncate part of it as the final set value, when the
> > > set value is greater than UINT_MAX, the set value will not match what we
> > > see because of the truncation. As follows:
> > > 
> > >    $ echo 4294967296 > /sys/fs/ext4/sda/mb_max_linear_groups
> > >    $ cat /sys/fs/ext4/sda/mb_max_linear_groups
> > >      0
> > > 
> > > So when the value set is outside the variable type range, -EINVAL is
> > > returned to avoid the inconsistency described above. In addition, a
> > > judgment is added to avoid setting s_resv_clusters less than 0.
> > > 
> > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > ---
> > >   fs/ext4/sysfs.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> > > index 6d332dff79dd..3671a8aaf4af 100644
> > > --- a/fs/ext4/sysfs.c
> > > +++ b/fs/ext4/sysfs.c
> > > @@ -104,7 +104,7 @@ static ssize_t reserved_clusters_store(struct ext4_sb_info *sbi,
> > >   	int ret;
> > >   	ret = kstrtoull(skip_spaces(buf), 0, &val);
> > > -	if (ret || val >= clusters)
> > > +	if (ret || val >= clusters || (s64)val < 0)
> > >   		return -EINVAL;
> > This looks a bit pointless, doesn't it? 'val' is u64, clusters is u64. We
> > know that val < clusters so how could (s64)val be < 0?
> When clusters is bigger than LLONG_MAX, (s64)val may be less than 0.
> Of course we don't have such a large storage device yet, so it's only
> theoretically possible to overflow here. But the previous patches in this
> patch set were intended to ensure that the values set via sysfs did not
> exceed the range of the variable type, so I've modified that here as well.

Well, my point was that the on disk format is limited to much less than
2^63 blocks. But I guess having the additional check does not matter.

> > > @@ -463,6 +463,8 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
> > >   		ret = kstrtoul(skip_spaces(buf), 0, &t);
> > >   		if (ret)
> > >   			return ret;
> > > +		if (t != (unsigned int)t)
> > > +			return -EINVAL;
> > >   		if (a->attr_ptr == ptr_ext4_super_block_offset)
> > >   			*((__le32 *) ptr) = cpu_to_le32(t);
> > >   		else
> > I kind of agree with Alexey that using kstrtouint() here instead would look
> > nicer. And it isn't like you have to define many new variables. You just
> > need unsigned long for attr_pointer_ul and unsigned int for
> > attr_pointer_ui.
>
> If we use both kstrtouint() and kstrtoul(), then we need to add
> kstrtouint() or kstrtoul() to each case, which would be a lot of
> duplicate code as follows:

Well, it is 5 more lines if I'm counting right :) (3x 3 lines of conversion
- 2x 2 lines of boundary checks). I kind of find it easier to oversee the
boundary checks when everything is together at each parameter. But frankly
this is a bit of nitpicking so if you feel strongly about this I won't
insist.

> static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
>                                        struct ext4_sb_info *sbi,
>                                        const char *buf, size_t len)
> {
>         int ret;
>         unsigned int t;
>         unsigned long lt;
>         void *ptr = calc_ptr(a, sbi);
> 
>         if (!ptr)
>                 return 0;
> 
>         switch (a->attr_id) {
>         case attr_group_prealloc:
>                 ret = kstrtouint(skip_spaces(buf), 0, &t);
>                 if (ret)
>                         return ret;
>                 if (t > sbi->s_clusters_per_group)
>                         return -EINVAL;
>                 return len;
>         case attr_pointer_pi:
>                 ret = kstrtouint(skip_spaces(buf), 0, &t);
>                 if (ret)
>                         return ret;
>                 if ((int)t < 0)
>                         return -EINVAL;
>                 return len;
>         case attr_pointer_ui:
>                 ret = kstrtouint(skip_spaces(buf), 0, &t);
>                 if (ret)
>                         return ret;
>                 if (t != (unsigned int)t)
>                         return -EINVAL;
		  ^^^ this can go away

>                 if (a->attr_ptr == ptr_ext4_super_block_offset)
>                         *((__le32 *) ptr) = cpu_to_le32(t);
>                 else
>                         *((unsigned int *) ptr) = t;
>                 return len;
>         case attr_pointer_ul:
>                 ret = kstrtoul(skip_spaces(buf), 0, &lt);
>                 if (ret)
>                         return ret;
>                 *((unsigned long *) ptr) = lt;
>                 return len;
>         }
>         return 0;
> 
> }
> 
> Also, both kstrtouint() and kstrtoul() are based on the kstrtoull()
> implementation, so it feels better to opencode kstrtoul() and
> kstrtouint() to reduce duplicate code.
> Why is it better to distinguish uint and ulong cases here?

Hopefully explained above :)


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