1. Patchew
  2. linux
  3. View series

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

Martin Kletzander posted 1 patch 1 month, 3 weeks ago 
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
[PATCH v4] x86/resctrl: Avoid overflow in MB settings in bw_validate()
Posted by Martin Kletzander 1 month, 3 weeks ago 
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 rounded 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.

Fixes: 8205a078ba78 ("x86/intel_rdt/mba_sc: Add schemata support")
Fixes: 6ce1560d35f6 ("x86/resctrl: Switch over to the resctrl mbps_val list")
Co-developed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Martin Kletzander <nert.pinx@gmail.com>
---
v4:
 - format the u32 variable properly
 - use better commit message

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 | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 50fa1fe9a073..200d89a64027 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,16 +42,21 @@ 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)) {
-		rdt_last_cmd_printf("MB value %ld out of range [%d,%d]\n", bw,
-				    r->membw.min_bw, r->default_ctrl);
+	/* 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 %u 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) {

base-commit: 7424fc6b86c8980a87169e005f5cd4438d18efe6
-- 
2.46.2
Re: [PATCH v4] x86/resctrl: Avoid overflow in MB settings in bw_validate()
Posted by Reinette Chatre 1 month, 3 weeks ago 
Hi Martin,

On 10/1/24 4:43 AM, Martin Kletzander wrote:
> 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 rounded 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.
> 
> Fixes: 8205a078ba78 ("x86/intel_rdt/mba_sc: Add schemata support")
> Fixes: 6ce1560d35f6 ("x86/resctrl: Switch over to the resctrl mbps_val list")
> Co-developed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Martin Kletzander <nert.pinx@gmail.com>
> ---

Thank you very much.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette
Re: [PATCH v4] x86/resctrl: Avoid overflow in MB settings in bw_validate()
Posted by Tony Luck 1 month, 3 weeks ago 
On Tue, Oct 01, 2024 at 01:43:56PM +0200, Martin Kletzander wrote:
> 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 rounded 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.
> 
> Fixes: 8205a078ba78 ("x86/intel_rdt/mba_sc: Add schemata support")
> Fixes: 6ce1560d35f6 ("x86/resctrl: Switch over to the resctrl mbps_val list")
> Co-developed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Martin Kletzander <nert.pinx@gmail.com>

Reviewed-by: Tony Luck <tony.luck@intel.com>

-Tony

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.