Re: [PATCH v4 1/2] selftests/resctrl: Fix schemata write error check

[Maciej Wieczór-Retman]

On 2023-09-28 at 14:25:23 -0700, Reinette Chatre wrote:
>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@xxxxxxxxx>,
>>>>   *    Fenghua Yu <fenghua.yu@xxxxxxxxx>
>>>>   */
>>>> +#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.

Right, sorry, then I guess initializing it to zero isn't a problem.

>> 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 tried compiling and running by passing a value with scanf to write()
(so the compiler doesn't know it's a negative one) just to check if
it behaves and write() was returning a negative one. But yes, the fact
it's unsigned means it's not a reliable way to catch this error.

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

Okay, thanks a lot for pointing all this out. I'll do just a less than
one check and move the initial value to zero.

-- 
Kind regards
Maciej Wieczór-Retman