Re: [PATCH v3] x86/resctrl: Avoid overflow in MB settings in bw_validate()

[Martin Kletzander <nert.pinx@gmail.com>]

On Thu, Sep 26, 2024 at 02:54:56PM +0200, Martin Kletzander wrote:
>On Tue, Sep 24, 2024 at 10:46:10AM -0700, Reinette Chatre wrote:
>>Hi Martin,
>>
>>On 9/24/24 1:53 AM, Martin Kletzander wrote:
>>> The memory bandwidth value was parsed as unsigned long, but later on
>>> rounded up and stored in u32.  That could result in an overflow,
>>> especially if resctrl is mounted with the "mba_MBps" option.
>>>
>>> Switch the variable right to u32 and parse it as such.
>>>
>>> Since the granularity and minimum bandwidth are not used when the
>>> software controller is used (resctrl is mounted with the "mba_MBps"),
>>> skip the rounding up as well and return early from bw_validate().
>>
>>Since this patch will flow via the tip tree the changelog needs
>>to meet the requirements documented in Documentation/process/maintainer-tip.rst
>>Here is an example how the changelog can be when taking into account
>>that context, problem, solution needs to be clearly separated with
>>everything written in imperative mood:
>>
>>	The resctrl schemata file supports specifying memory bandwidth
>>	associated with the Memory Bandwidth Allocation (MBA) feature
>>	via a percentage (this is the default) or bandwidth in MiBps
>>	(when resctrl is mounted with the "mba_MBps" option). The allowed
>>	range for the bandwidth percentage is from
>>	/sys/fs/resctrl/info/MB/min_bandwidth to 100, using a granularity
>>	of /sys/fs/resctrl/info/MB/bandwidth_gran. The supported range for
>>	the MiBps bandwidth is 0 to U32_MAX.
>>
>>	There are two issues with parsing of MiBps memory bandwidth:
>>	* The user provided MiBps is mistakenly round up to the granularity
>>	  that is unique to percentage input.
>>	* The user provided MiBps is parsed using unsigned long (thus accepting
>>	  values up to ULONG_MAX), and then assigned to u32 that could result in
>>	  overflow.
>>
>>	Do not round up the MiBps value and parse user provided bandwidth as
>>	the u32 it is intended to be. Use the appropriate kstrtou32() that
>>	can detect out of range values.
>>
>
>Great, can I use your commit message then?  I wouldn't be able to write
>it as nicely =)
>
>>
>>This needs "Fixes" tags. Looks like the following are appropriate:
>>Fixes: 8205a078ba78 ("x86/intel_rdt/mba_sc: Add schemata support")
>>Fixes: 6ce1560d35f6 ("x86/resctrl: Switch over to the resctrl mbps_val list")
>>
>
>It seems to me like this should've been handled in commit 8205a078ba78
>("x86/intel_rdt/mba_sc: Add schemata support") which added support for
>mba_sc and kept the rounding up of the value while skipping the range
>validation.
>
>>> Signed-off-by: Martin Kletzander <nert.pinx@gmail.com>
>>> Co-developed-by: Reinette Chatre <reinette.chatre@intel.com>
>>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>>
>>Please place your SoB at the end. For details about tag ordering
>>you can refer to section "Ordering of commit tags" in
>>Documentation/process/maintainer-tip.rst
>>
>
>I'll go through that docs too, thanks.
>
>>
>>>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 19 ++++++++++++-------
>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> index 50fa1fe9a073..53defc5a6784 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>>> @@ -29,10 +29,10 @@
>>>   * hardware. The allocated bandwidth percentage is rounded to the next
>>>   * control step available on the hardware.
>>>   */
>>> -static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
>>> +static bool bw_validate(char *buf, u32 *data, struct rdt_resource *r)
>>>  {
>>> -	unsigned long bw;
>>>  	int ret;
>>> +	u32 bw;
>>>
>>>  	/*
>>>  	 * Only linear delay values is supported for current Intel SKUs.
>>> @@ -42,14 +42,19 @@ static bool bw_validate(char *buf, unsigned long *data, struct rdt_resource *r)
>>>  		return false;
>>>  	}
>>>
>>> -	ret = kstrtoul(buf, 10, &bw);
>>> +	ret = kstrtou32(buf, 10, &bw);
>>>  	if (ret) {
>>> -		rdt_last_cmd_printf("Non-decimal digit in MB value %s\n", buf);
>>> +		rdt_last_cmd_printf("Invalid MB value %s\n", buf);
>>>  		return false;
>>>  	}
>>>
>>> -	if ((bw < r->membw.min_bw || bw > r->default_ctrl) &&
>>> -	    !is_mba_sc(r)) {
>>> +	/* Nothing else to do if software controller is enabled. */
>>> +	if (is_mba_sc(r)) {
>>> +		*data = bw;
>>> +		return true;
>>> +	}
>>> +
>>> +	if (bw < r->membw.min_bw || bw > r->default_ctrl) {
>>>  		rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
>>>  				    r->membw.min_bw, r->default_ctrl);
>>
>>By now you may have noticed the lkp report [1] catching an issue with my
>>code snippet. Could you please take a look? Seems that %u would be appropriate.
>>
>
>Yes, I finally got to it and %u should definitely work.  I wanted to go
>with what seems more appropriate, PRIu32, but this file does not have
>access to it and in order to include inttypes I would have to change the
>Makefile too, which seems too much of a change given it takes me this
>many tries already :)
>
>However if PRIu32 is preferred I have no problem adjusting the build
>process as well.
>

Disregard this, I found out the only uses of PRIu32 are in code that has
access to glibc (scripts, tools, etc.), sorry for the noise.

>>>  		return false;
>>> @@ -65,7 +70,7 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
>>>  	struct resctrl_staged_config *cfg;
>>>  	u32 closid = data->rdtgrp->closid;
>>>  	struct rdt_resource *r = s->res;
>>> -	unsigned long bw_val;
>>> +	u32 bw_val;
>>>
>>>  	cfg = &d->staged_config[s->conf_type];
>>>  	if (cfg->have_new_ctrl) {
>>
>>Reinette
>>
>>[1] https://lore.kernel.org/all/202409250046.1Kk0NXVZ-lkp@intel.com/
>>
>>