Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Thu, 21 Sep 2023 19:18:43 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     kernel test robot <oliver.sang@intel.com>
Cc:     oe-lkp@lists.linux.dev, lkp@intel.com,
        linux-kernel@vger.kernel.org, Luis Chamberlain <mcgrof@kernel.org>
Subject: Re: [PATCH v1 1/6] params: Use sysfs_emit() to instead of scnprintf()
Message-ID: <ZQxs445+AIBnSd8w@smile.fi.intel.com>
References: <20230912150551.401537-1-andriy.shevchenko@linux.intel.com>
 <202309211632.67e4c1e0-oliver.sang@intel.com>
 <ZQxjBkFZV5T6BELA@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZQxjBkFZV5T6BELA@smile.fi.intel.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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