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

[Reinette Chatre <reinette.chatre@intel.com>]

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.                                                 


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")   
 
> 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


>  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.

>  		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/