| Message ID | 20250410-fix_fs-v1-1-7c14ccc8ebaa@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: bug fixes | expand |
On Thu, 10 Apr 2025 19:45:27 +0800, Zijun Hu wrote: > fs_name() has @index as unsigned int, so there is underflow risk for > operation '@index--'. > > Fix by breaking the for loop when '@index == 0' which is also more proper > than '@index <= 0' for unsigned integer comparison. > > > [...] Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree. Patches in the vfs-6.16.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-6.16.misc [1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name() https://git.kernel.org/vfs/vfs/c/d319af11e9a1
On Thu, Apr 10, 2025 at 07:45:27PM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > fs_name() has @index as unsigned int, so there is underflow risk for > operation '@index--'. > > Fix by breaking the for loop when '@index == 0' which is also more proper > than '@index <= 0' for unsigned integer comparison. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- This is honestly not worth the effort thinking about. I'm going to propose that we remove this system call or at least switch the default to N. Nobody uses this anymore I'm pretty sure. > fs/filesystems.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/fs/filesystems.c b/fs/filesystems.c > index 58b9067b2391ce814e580709b518b405e0f9cb8a..95e5256821a53494d88f496193305a2e50e04444 100644 > --- a/fs/filesystems.c > +++ b/fs/filesystems.c > @@ -156,15 +156,19 @@ static int fs_index(const char __user * __name) > static int fs_name(unsigned int index, char __user * buf) > { > struct file_system_type * tmp; > - int len, res; > + int len, res = -EINVAL; > > read_lock(&file_systems_lock); > - for (tmp = file_systems; tmp; tmp = tmp->next, index--) > - if (index <= 0 && try_module_get(tmp->owner)) > + for (tmp = file_systems; tmp; tmp = tmp->next, index--) { > + if (index == 0) { > + if (try_module_get(tmp->owner)) > + res = 0; > break; > + } > + } > read_unlock(&file_systems_lock); > - if (!tmp) > - return -EINVAL; > + if (res) > + return res; > > /* OK, we got the reference, so we can safely block */ > len = strlen(tmp->name) + 1; > > -- > 2.34.1 >
On 2025/4/11 22:35, Christian Brauner wrote: >> Fix by breaking the for loop when '@index == 0' which is also more proper >> than '@index <= 0' for unsigned integer comparison. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- > This is honestly not worth the effort thinking about. > I'm going to propose that we remove this system call or at least switch > the default to N. Nobody uses this anymore I'm pretty sure Sound good. i just started looking at FS code (^^).
Zijun Hu <zijun_hu@icloud.com> wrote: > fs_name() has @index as unsigned int, so there is underflow risk for > operation '@index--'. > > Fix by breaking the for loop when '@index == 0' which is also more proper > than '@index <= 0' for unsigned integer comparison. There isn't really a risk. The list walked by "tmp" and the checks that this is or is not NULL will prevent a problem. I also feel that breaking out of the loop with "<= 0" - even if the variable is unsigned - is safer, on the off chance that someone in the future changes the signedness of the variable. David
On 2025/4/11 23:34, David Howells wrote: >> Fix by breaking the for loop when '@index == 0' which is also more proper >> than '@index <= 0' for unsigned integer comparison. > There isn't really a risk. The list walked by "tmp" and the checks that this > is or is not NULL will prevent a problem. > no fixes tag is added and just improve code logic a bit since there is no reason to continue the loop when @index reach 0. > I also feel that breaking out of the loop with "<= 0" - even if the variable > is unsigned - is safer, on the off chance that someone in the future changes > the signedness of the variable. for parameter @index representing filesystem index. unsigned integer type may be better than signed.
diff --git a/fs/filesystems.c b/fs/filesystems.c index 58b9067b2391ce814e580709b518b405e0f9cb8a..95e5256821a53494d88f496193305a2e50e04444 100644 --- a/fs/filesystems.c +++ b/fs/filesystems.c @@ -156,15 +156,19 @@ static int fs_index(const char __user * __name) static int fs_name(unsigned int index, char __user * buf) { struct file_system_type * tmp; - int len, res; + int len, res = -EINVAL; read_lock(&file_systems_lock); - for (tmp = file_systems; tmp; tmp = tmp->next, index--) - if (index <= 0 && try_module_get(tmp->owner)) + for (tmp = file_systems; tmp; tmp = tmp->next, index--) { + if (index == 0) { + if (try_module_get(tmp->owner)) + res = 0; break; + } + } read_unlock(&file_systems_lock); - if (!tmp) - return -EINVAL; + if (res) + return res; /* OK, we got the reference, so we can safely block */ len = strlen(tmp->name) + 1;