Message-ID: <6324595f-99e6-4eb4-ae40-af1bb765079c@intel.com>
Date: Thu, 26 Sep 2024 09:27:54 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Martin Kletzander <nert.pinx@...il.com>
CC: Fenghua Yu <fenghua.yu@...el.com>, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	<dave.hansen@...ux.intel.com>, <x86@...nel.org>, "H. Peter Anvin"
	<hpa@...or.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] x86/resctrl: Avoid overflow in MB settings in
 bw_validate()

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

Powered by blists - more mailing lists