| Message ID | 1605341356-11872-1-git-send-email-yangyicong@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] libfs: fix error cast of negative value in simple_attr_write() | expand |
On Sat, Nov 14, 2020 at 04:09:16PM +0800, Yicong Yang wrote: > The attr->set() receive a value of u64, but simple_strtoll() is used > for doing the conversion. It will lead to the error cast if user inputs > a negative value. > > Use kstrtoull() instead of simple_strtoll() to convert a string got > from the user to an unsigned value. The former will return '-EINVAL' if > it gets a negetive value, but the latter can't handle the situation > correctly. Make 'val' unsigned long long as what kstrtoull() takes, this > will eliminate the compile warning on no 64-bit architectures. > > Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines") > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > Change since v1: > - address the compile warning for non-64 bit platform. > Change since v2: > Link: https://lore.kernel.org/linux-fsdevel/1605000324-7428-1-git-send-email-yangyicong@hisilicon.com/ > - make 'val' unsigned long long and mentioned in the commit > Link: https://lore.kernel.org/linux-fsdevel/1605261369-551-1-git-send-email-yangyicong@hisilicon.com/ Belated error report on this. Some validation team just moved to v5.10 and found their error injection scripts no longer work. They have been using: # echo $((-1 << 12)) > /sys/kernel/debug/apei/einj/param2 to write the mask value 0xfffffffffffff000 for many years ... but now writing a negative value (-4096) to this file gives an EINVAL error. Maybe they've been taking advantage of a bug all this time? The comment for debugfs_create_x64() says it is for reading/writing an unsigned value. But when a bug fix breaks user code ... then we are supposed to ask whether that bug is actually a feature. If there was a debugfs_create_s64() I might just fix einj.c to use that ... but there isn't :-( -Tony
On 2021/6/4 0:09, Luck, Tony wrote: > On Sat, Nov 14, 2020 at 04:09:16PM +0800, Yicong Yang wrote: >> The attr->set() receive a value of u64, but simple_strtoll() is used >> for doing the conversion. It will lead to the error cast if user inputs >> a negative value. >> >> Use kstrtoull() instead of simple_strtoll() to convert a string got >> from the user to an unsigned value. The former will return '-EINVAL' if >> it gets a negetive value, but the latter can't handle the situation >> correctly. Make 'val' unsigned long long as what kstrtoull() takes, this >> will eliminate the compile warning on no 64-bit architectures. >> >> Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines") >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> Change since v1: >> - address the compile warning for non-64 bit platform. >> Change since v2: >> Link: https://lore.kernel.org/linux-fsdevel/1605000324-7428-1-git-send-email-yangyicong@hisilicon.com/ >> - make 'val' unsigned long long and mentioned in the commit >> Link: https://lore.kernel.org/linux-fsdevel/1605261369-551-1-git-send-email-yangyicong@hisilicon.com/ > > Belated error report on this. Some validation team just moved to > v5.10 and found their error injection scripts no longer work. > > They have been using: > > # echo $((-1 << 12)) > /sys/kernel/debug/apei/einj/param2 > > to write the mask value 0xfffffffffffff000 for many years ... but > now writing a negative value (-4096) to this file gives an EINVAL error. > ok. i didn't know this usage. I was using debugfs for my driver but found I cannot figure out whether user has entered a negative value. > Maybe they've been taking advantage of a bug all this time? The > comment for debugfs_create_x64() says it is for reading/writing > an unsigned value. But when a bug fix breaks user code ... then > we are supposed to ask whether that bug is actually a feature. > ok. sounds reasonable. > If there was a debugfs_create_s64() I might just fix einj.c to > use that ... but there isn't :-( yes, a debugfs_create_s64() seems better for this case. But it's okay for me to revert this fix if we regard this bug as a feature. Thanks Yicong
diff --git a/fs/libfs.c b/fs/libfs.c index fc34361..7124c2e 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -959,7 +959,7 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf, size_t len, loff_t *ppos) { struct simple_attr *attr; - u64 val; + unsigned long long val; size_t size; ssize_t ret; @@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf, goto out; attr->set_buf[size] = '\0'; - val = simple_strtoll(attr->set_buf, NULL, 0); + ret = kstrtoull(attr->set_buf, 0, &val); + if (ret) + goto out; ret = attr->set(attr->data, val); if (ret == 0) ret = len; /* on success, claim we got the whole input */
The attr->set() receive a value of u64, but simple_strtoll() is used for doing the conversion. It will lead to the error cast if user inputs a negative value. Use kstrtoull() instead of simple_strtoll() to convert a string got from the user to an unsigned value. The former will return '-EINVAL' if it gets a negetive value, but the latter can't handle the situation correctly. Make 'val' unsigned long long as what kstrtoull() takes, this will eliminate the compile warning on no 64-bit architectures. Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines") Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- Change since v1: - address the compile warning for non-64 bit platform. Change since v2: Link: https://lore.kernel.org/linux-fsdevel/1605000324-7428-1-git-send-email-yangyicong@hisilicon.com/ - make 'val' unsigned long long and mentioned in the commit Link: https://lore.kernel.org/linux-fsdevel/1605261369-551-1-git-send-email-yangyicong@hisilicon.com/ fs/libfs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)