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

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

Posted by Martin Kletzander 2 months ago

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

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>
---
v3:
 - use u32 right away without going through unsigned long

v2:
 - actually save the value in the output parameter @data

 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);
 		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) {
-- 
2.46.1

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

Posted by kernel test robot 2 months ago

Hi Martin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/x86/core]
[also build test WARNING on linus/master v6.11 next-20240924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Kletzander/x86-resctrl-Avoid-overflow-in-MB-settings-in-bw_validate/20240924-165510
base:   tip/x86/core
patch link:    https://lore.kernel.org/r/8d028c5f6f23e92fb83cbf20599366e896abc5b5.1727167989.git.nert.pinx%40gmail.com
patch subject: [PATCH v3] x86/resctrl: Avoid overflow in MB settings in bw_validate()
config: x86_64-buildonly-randconfig-005-20240924 (https://download.01.org/0day-ci/archive/20240925/202409250124.D1gI7Lzj-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240925/202409250124.D1gI7Lzj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409250124.D1gI7Lzj-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/x86/kernel/cpu/resctrl/ctrlmondata.c:58:62: warning: format specifies type 'long' but the argument has type 'u32' (aka 'unsigned int') [-Wformat]
      58 |                 rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
         |                                               ~~~                          ^~
         |                                               %u
   1 warning generated.


vim +58 arch/x86/kernel/cpu/resctrl/ctrlmondata.c

60ec2440c63dea arch/x86/kernel/cpu/intel_rdt_schemata.c    Tony Luck         2016-10-28  25  
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  26  /*
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  27   * Check whether MBA bandwidth percentage value is correct. The value is
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  28   * checked against the minimum and max bandwidth values specified by the
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  29   * hardware. The allocated bandwidth percentage is rounded to the next
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  30   * control step available on the hardware.
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  31   */
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  32  static bool bw_validate(char *buf, u32 *data, struct rdt_resource *r)
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  33  {
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  34  	int ret;
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  35  	u32 bw;
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  36  
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  37  	/*
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  38  	 * Only linear delay values is supported for current Intel SKUs.
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  39  	 */
41215b7947f1b1 arch/x86/kernel/cpu/resctrl/ctrlmondata.c   James Morse       2020-07-08  40  	if (!r->membw.delay_linear && r->membw.arch_needs_linear) {
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  41  		rdt_last_cmd_puts("No support for non-linear MB domains\n");
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  42  		return false;
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  43  	}
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  44  
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  45  	ret = kstrtou32(buf, 10, &bw);
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  46  	if (ret) {
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  47  		rdt_last_cmd_printf("Invalid MB value %s\n", buf);
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  48  		return false;
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  49  	}
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  50  
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  51  	/* Nothing else to do if software controller is enabled. */
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  52  	if (is_mba_sc(r)) {
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  53  		*data = bw;
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  54  		return true;
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  55  	}
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  56  
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  57  	if (bw < r->membw.min_bw || bw > r->default_ctrl) {
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25 @58  		rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  59  				    r->membw.min_bw, r->default_ctrl);
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  60  		return false;
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  61  	}
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  62  
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  63  	*data = roundup(bw, (unsigned long)r->membw.bw_gran);
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  64  	return true;
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  65  }
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  66  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

Posted by Reinette Chatre 2 months ago

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/

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

Posted by Martin Kletzander 2 months ago

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.

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

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

Posted by Reinette Chatre 2 months ago

Hi Martin,

On 9/26/24 5:54 AM, 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 =)

Sure.

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

Right. That commit additionally suffers from the overflow problem by, after
rounding up the value, assigning the unsigned long result to a u32 (struct
rdt_domain.newctrl).

I added 6ce1560d35f6, not because of the rounding issue, but instead 
of it switching the destination of assignment to struct rdt_domain.mbps_val,
which also happens to be a u32.

I included both commits with the goal to help anybody that may be looking
at backporting this fix.

Reinette

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

Posted by Martin Kletzander 2 months ago

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

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

Posted by kernel test robot 2 months ago

Hi Martin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/x86/core]
[also build test WARNING on linus/master v6.11 next-20240924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Kletzander/x86-resctrl-Avoid-overflow-in-MB-settings-in-bw_validate/20240924-165510
base:   tip/x86/core
patch link:    https://lore.kernel.org/r/8d028c5f6f23e92fb83cbf20599366e896abc5b5.1727167989.git.nert.pinx%40gmail.com
patch subject: [PATCH v3] x86/resctrl: Avoid overflow in MB settings in bw_validate()
config: x86_64-buildonly-randconfig-001-20240924 (https://download.01.org/0day-ci/archive/20240925/202409250046.1Kk0NXVZ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240925/202409250046.1Kk0NXVZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409250046.1Kk0NXVZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   arch/x86/kernel/cpu/resctrl/ctrlmondata.c: In function 'bw_validate':
>> arch/x86/kernel/cpu/resctrl/ctrlmondata.c:58:49: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'u32' {aka 'unsigned int'} [-Wformat=]
      58 |                 rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
         |                                               ~~^                          ~~
         |                                                 |                          |
         |                                                 long int                   u32 {aka unsigned int}
         |                                               %d


vim +58 arch/x86/kernel/cpu/resctrl/ctrlmondata.c

60ec2440c63dea arch/x86/kernel/cpu/intel_rdt_schemata.c    Tony Luck         2016-10-28  25  
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  26  /*
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  27   * Check whether MBA bandwidth percentage value is correct. The value is
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  28   * checked against the minimum and max bandwidth values specified by the
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  29   * hardware. The allocated bandwidth percentage is rounded to the next
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  30   * control step available on the hardware.
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  31   */
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  32  static bool bw_validate(char *buf, u32 *data, struct rdt_resource *r)
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  33  {
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  34  	int ret;
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  35  	u32 bw;
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  36  
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  37  	/*
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  38  	 * Only linear delay values is supported for current Intel SKUs.
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  39  	 */
41215b7947f1b1 arch/x86/kernel/cpu/resctrl/ctrlmondata.c   James Morse       2020-07-08  40  	if (!r->membw.delay_linear && r->membw.arch_needs_linear) {
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  41  		rdt_last_cmd_puts("No support for non-linear MB domains\n");
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  42  		return false;
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  43  	}
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  44  
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  45  	ret = kstrtou32(buf, 10, &bw);
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  46  	if (ret) {
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  47  		rdt_last_cmd_printf("Invalid MB value %s\n", buf);
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  48  		return false;
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  49  	}
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  50  
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  51  	/* Nothing else to do if software controller is enabled. */
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  52  	if (is_mba_sc(r)) {
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  53  		*data = bw;
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  54  		return true;
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  55  	}
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  56  
be99ce3b7dd7ba arch/x86/kernel/cpu/resctrl/ctrlmondata.c   Martin Kletzander 2024-09-24  57  	if (bw < r->membw.min_bw || bw > r->default_ctrl) {
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25 @58  		rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  59  				    r->membw.min_bw, r->default_ctrl);
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  60  		return false;
c377dcfbee808e arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c Tony Luck         2017-09-25  61  	}
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  62  
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  63  	*data = roundup(bw, (unsigned long)r->membw.bw_gran);
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  64  	return true;
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  65  }
64e8ed3d4a6dcd arch/x86/kernel/cpu/intel_rdt_schemata.c    Vikas Shivappa    2017-04-07  66  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki