| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240223115443.spaztzcv7llmfl77@quack3>
Date: Fri, 23 Feb 2024 12:54:43 +0100
From: Jan Kara <jack@...e.cz>
To: Baokun Li <libaokun1@...wei.com>
Cc: Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org, tytso@....edu,
adilger.kernel@...ger.ca, ritesh.list@...il.com,
linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
yangerkun@...wei.com, chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH 1/7] ext4: avoid overflow when setting values via sysfs
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@...wei.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, <);
> 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@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists