Received-SPF: pass (google.com: domain of linux-ext4+bounces-1375-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 23 Feb 2024 12:54:43 +0100
From: Jan Kara <jack@suse.cz>
To: Baokun Li <libaokun1@huawei.com>
Cc: Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org, tytso@mit.edu,
	adilger.kernel@dilger.ca, ritesh.list@gmail.com,
	linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
	yangerkun@huawei.com, chengzhihao1@huawei.com, yukuai3@huawei.com
Subject: Re: [PATCH 1/7] ext4: avoid overflow when setting values via sysfs
Message-ID: <20240223115443.spaztzcv7llmfl77@quack3>
References: <20240126085716.1363019-1-libaokun1@huawei.com>
 <20240126085716.1363019-2-libaokun1@huawei.com>
 <20240213160554.35cpsfqqeqpgtux2@quack3>
 <81081ec9-3aab-ecd1-c2f6-9a3835ea4fda@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <81081ec9-3aab-ecd1-c2f6-9a3835ea4fda@huawei.com>

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