Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <efcfa0a3-3350-74af-5aa7-8306ab206b56@intel.com>
Date:   Thu, 28 Sep 2023 14:25:23 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.15.1
Subject: Re: [PATCH v4 1/2] selftests/resctrl: Fix schemata write error check
Content-Language: en-US
To:     =?UTF-8?Q?Maciej_Wiecz=c3=b3r-Retman?= 
        <maciej.wieczor-retman@intel.com>
CC:     Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>,
        <ilpo.jarvinen@linux.intel.com>, <linux-kernel@vger.kernel.org>,
        <linux-kselftest@vger.kernel.org>
References: <cover.1695369120.git.maciej.wieczor-retman@intel.com>
 <5e17ed39ffb8d6bd530c057aa04e3ffb997573a9.1695369120.git.maciej.wieczor-retman@intel.com>
 <322fb9ff-f3f4-45df-9b3f-524125ca6101@intel.com>
 <mrhrlwtpquqqyzznzqru2m5tfbz7cmtk76zxtamq76ssqthz6g@njfi4qvjclih>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <mrhrlwtpquqqyzznzqru2m5tfbz7cmtk76zxtamq76ssqthz6g@njfi4qvjclih>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R0ZxcmFmVFE2SFlmdEw2Z0psN0JETys3ZE00QkNuZEtDeGZwSkFuaUYvd3ZJ?=
 =?utf-8?B?V01TbWk0R1I0b1IrYUF2RTBYalhBRjhHM1c3MGcvUDRJVXNHTlRHYmNoM0xh?=
 =?utf-8?B?T3B0amxuKy9aMWlia1Z2Nk5mOUt3bmNhRWl6MldOM3BjbGZQMWEvdWIxSFZm?=
 =?utf-8?B?ODVFQy8yczJpajA2WC9rYXQxQis1dENocGE5NCsyMDhkMEtDdEJyTUxsTWhz?=
 =?utf-8?B?QUpGTi9hd21mMUI0T1E0SmdaU3FpTFpoYm9mckpnWVRjbHQ0SU53Zy9kSXZr?=
 =?utf-8?B?T1dKRkdtVGh1SkFSekZ4QUQ0S0xMeFdZQ25uNnQ2d2x1bTdZNElkNEtuSlIx?=
 =?utf-8?B?aE1BUklsK3JoYU9wTlpxSFo0TWErUnRONG1vcEpuSGdHc1VONnFNYzA5VG1i?=
 =?utf-8?B?R3JWMkRjSE1TZ1MxRm9RQmpqM3g1SjFhdzlCNXVNVXZDVmMxUEpwUks2dk9M?=
 =?utf-8?B?T2RvRVBIajVhN1k1UHZibk94SUkySmd1d2lnak5vN045ZXdhZjhlRHlCS25p?=
 =?utf-8?B?d0RmY2pKRUdwVzRLWGZUeCtKM3NJbjhoUUN6V3VOdzYwMFlveDhiME5QOWVO?=
 =?utf-8?B?RUt5UVRSM2F2bFN2RmtIT0FQeUtmYk4rcmU4Y0NicW83My96cGRjbFJvNXpr?=
 =?utf-8?B?S2QzMTMzdi84WVFhYy9OYkhmazBld2d1NitENVYzenh3K3h0anNXMTNEZit3?=
 =?utf-8?B?OXNzanJ0YUsxenNpWXVWbmZ0L1BNVUxXcmd0cGRENzNObUtTZFd1Zlo0SU4x?=
 =?utf-8?B?TVIyRUtZblBBZ09QNmM0SFFRYWl0MXFlUThHc2tqUzhSN3h5dTVVcnRJbTlS?=
 =?utf-8?B?VDJ1VTNrdGNmR3VLbVJuTWNlMExDZVZDNWt5aVo4QmljMnA5VEV5YlZXQ3FE?=
 =?utf-8?B?RXVya0YyKzBKQ05ER2gvRnNhL0haamRJelU2ZG13RHFjZzNTQ3ZrMGZDRG5R?=
 =?utf-8?B?VlN0UDRROEJNNEtHVE43d3BZUXpSZ2pLODltT1VabVZVTzQ4R0hnTUh1STdn?=
 =?utf-8?B?RUtLKzllRHZOUndBaUNPeTVDcnIxYURHUVkxSUdDY1pjYUdnYzRHZlZackNp?=
 =?utf-8?B?WCtnREpVNjV2ZFNYTE9OTmx6NDB6eGJCMjEyL2h6MHJRT0Z4WGdCNG90NzFi?=
 =?utf-8?B?VkdEcDZvczA0UVNQVnZEUG8rUllFWERvOFFYZERIQzE0L25BTDV6YUM1WHN1?=
 =?utf-8?B?ekpZVm0zUjM2a2xlb1BGaEtSRWJXNm8zeUttMWJtVXhnbktyL24rSm45SnFN?=
 =?utf-8?B?OXlCMk1sREJXS0lXLzdkZWo3amtKTlZuN1hwOTJnMm9TS1lSTXM4SFZwNlVZ?=
 =?utf-8?B?SmpvamVJNG5meElKVDk5NVlzVEd6WDE0TUFaTlFWdmpxTzRNQm9GOXczV2gz?=
 =?utf-8?B?alR6VUFoNjhQYURUY2c3UXV3TWVPcXZEZm92M05MQ0NsanB1R2doWmQxaDJm?=
 =?utf-8?B?bmtIcGUwVTB2cUNjU0ExYWkwZFhuVmhyT25HSURDaFRpUmkzQjR6WE1OSnRS?=
 =?utf-8?B?OTdWQU52elo3Q3ZjN1JnNkVnb21USXl1WkZxY081TVRTQkl1NDMydlpPazZq?=
 =?utf-8?B?d25WWmFKa2VoQmQ5TGdiN25ML0JDOWtmSXI0R3I4NGEzdVNCVHB6YUJnRUdm?=
 =?utf-8?B?MlBFWEVROWtYTkJXUmk1Z3BDWjlvd1pINlZ3UTUxWXdSQzVXVlFtNWlCMW85?=
 =?utf-8?B?QmZJVEZwSVAwQksvdG9iYTdhNzR5ZDFES3pkVjZNV0VQcktCeUJvK21XSDRC?=
 =?utf-8?B?eHphd1Z5ZlczaEFmU1lpTlZYN3RNSUJTekdYeFkyandKYkg5aXA5Tm5udHdD?=
 =?utf-8?B?bGk0TUdOVTI3MzkxUEd2dVFtNmtNS2xMUXBLWUQwcVpzVEYreDZEcXdzV081?=
 =?utf-8?B?Z1luak9nK1BCdFJEODFYYVJNYWZNWVhqc0hydmhqREVCZGVGSzRWL3lRS2p0?=
 =?utf-8?B?RHRNM1QzTGZhN0FQdjUwOFNMTGsxZ2ZEbk4rT3E5SCtFZStva2pXY0t4dTJH?=
 =?utf-8?B?U0sweXlPRzRWWHI2ck52aHpGQVFKaTVlRStUbEk0K0h0bjIzeklXNUg2TVpl?=
 =?utf-8?B?WnRvU2VHcVN3bDdWSnVpaWc2bHByeG9pK3lvS2VzQnNsQUtHSkRlM2lTbTB6?=
 =?utf-8?B?UWM0ajZzazRmYW5mTDJ5WENweVpUQUU4M0x4VXBCOFJjWnEzM2JnWUxVK1VG?=
 =?utf-8?B?Qnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 46c28517-d619-420a-406e-08dbc0696eaa
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2023 21:25:26.5313
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WPITkrLKFIwAikqciFfQ9N+gHoc/ebeylcxp8cBhkYMf437GC7ymkUc3z/59w31cN5lnKDZQnCeKdgZLycTsQzOuIsHY2ovHfxHEeIP6Hdc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5616
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Thu, 28 Sep 2023 14:25:45 -0700 (PDT)

Hi Maciej,

On 9/27/2023 11:46 PM, Maciej Wieczór-Retman wrote:
> On 2023-09-27 at 15:15:06 -0700, Reinette Chatre wrote:
>> On 9/22/2023 1:10 AM, Maciej Wieczor-Retman wrote:

>>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>>> index 3a8111362d26..edc8fc6e44b0 100644
>>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>>> @@ -8,6 +8,7 @@
>>>   *    Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
>>>   *    Fenghua Yu <fenghua.yu@intel.com>
>>>   */
>>> +#include <fcntl.h>
>>>  #include <limits.h>
>>>  
>>>  #include "resctrl.h"
>>> @@ -490,9 +491,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>>>   */
>>>  int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>>>  {
>>> -	char controlgroup[1024], schema[1024], reason[64];
>>> -	int resource_id, ret = 0;
>>> -	FILE *fp;
>>> +	char controlgroup[1024], schema[1024], reason[128];
>>> +	int resource_id, fd, schema_len = -1, ret = 0;
>>
>> I am trying to understand the schema_len initialization. Could
>> you please elaborate why you chose -1? I'm a bit concerned with
>> the robustness here with it being used as an unsigned integer
>> in write() and also the negative array index later.
> 
> My idea was that if the initial value for schema_len was 0, then if
> resctrl_val wouldn't equal any of MBA_STR, MBM_STR, CAT_STR, CMT_STR

Ensuring that resctrl_val is equal to one of these seems to be the
first thing write_schemata() does.

> values schema_len would stay zero and write nothing.

Your alternative writes "-1". write() is declared as:
	ssize_t write(int fd, const void *buf, size_t count);

note that "count" is size_t, which is an unsigned value. Providing
it -1 is thus a very large number and likely to cause overflow. In fact
if I even try to compile a program where the compiler can figure out
count will be -1 it fails the compile (stringop-overflow).
 
> I think it would be difficult to debug such an error because even later
> in ksft_print_msg the requested schema would get printed as if there was
> no error. In the case I mentioned above the function will just error out
> which I assume could be helpful.

You seem to rely on write() to cleanly catch giving it bad data.

> Other solutions that can accomplish the same goal would be checking
> write() not only for negative values but also for zero (since in
> here this is pretty much an error). Or checking schema_len for only
> positive values after the block of code where it gets assigned a
> value from sprintf.
> 
> Are any of the above safer or more logical in your opinion?

There is no error checking on schema_len. After it has been initialized it
can be checked for errors and write_schemata() can be exited immediately if
an error was encountered without attempting the write().

Reinette