Messages in this thread | | | Date | Thu, 21 Sep 2023 19:18:43 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v1 1/6] params: Use sysfs_emit() to instead of scnprintf() |
| |
On Thu, Sep 21, 2023 at 06:36:38PM +0300, Andy Shevchenko wrote: > On Thu, Sep 21, 2023 at 09:34:13PM +0800, kernel test robot wrote: > > > > Hello, > > > > kernel test robot noticed "WARNING:at_fs/sysfs/file.c:#sysfs_emit" on: > > > > commit: d4004295e5502a1eb3e361e97ea4dd1686046af6 ("[PATCH v1 1/6] params: Use sysfs_emit() to instead of scnprintf()") > > url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/params-Introduce-the-param_unknown_fn-type/20230912-231033 > > base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 0bb80ecc33a8fb5a682236443c1e740d5c917d1d > > patch link: https://lore.kernel.org/all/20230912150551.401537-1-andriy.shevchenko@linux.intel.com/ > > patch subject: [PATCH v1 1/6] params: Use sysfs_emit() to instead of scnprintf() > > > > in testcase: trinity > > version: trinity-i386-abe9de86-1_20230429 > > with following parameters: > > > > runtime: 300s > > group: group-04 > > nr_groups: 5 > > > > what we observed is this issue doesn't always happen. we run the test upon > > this commit almost 500 times, it happened 42 times. > > however, the parent keeps clean. > > > > v6.6-rc1 d4004295e5502a1eb3e361e97ea > > ---------------- --------------------------- > > fail:runs %reproduction fail:runs > > | | | > > :497 8% 42:496 dmesg.EIP:sysfs_emit > > :497 8% 42:496 dmesg.WARNING:at_fs/sysfs/file.c:#sysfs_emit > > Cool! I will check this, thank you for the report.
Oh, my gosh... This reveals a nice overflow bug for some getters that expect buffer to be PAGE_SIZE, but an array can be bigger than that.
So, basically this is a flaw in param_array_get() which is a wrapper on top of getter and calls ->get() without any proper alignment or buffer size guarantee!
While ->get() is by nature suppose to get an aligned buffer of PAGE_SIZE.
Ideally we need to have an additional ->get_array_element() callback which will take an offset. Less intrusive one is to have an allocated buffer of PAGE_SIZE in the param_array_get() and ->get() to it, then copy to the real one with the offset. Any other proposals?
Luis, which solution would you prefer?
-- With Best Regards, Andy Shevchenko
| |