Re: [PATCH V3 05/15] selftests/resctrl: Protect against array overflow when reading strings

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

On Thu, 17 Oct 2024, Reinette Chatre wrote:

> resctrl selftests discover system properties via a variety of sysfs files.
> The MBM and MBA tests need to discover the event and umask with which to
> configure the performance event used to measure read memory bandwidth.
> This is done by parsing the contents of
> /sys/bus/event_source/devices/uncore_imc_<imc instance>/events/cas_count_read
> Similarly, the resctrl selftests discover the cache size via
> /sys/bus/cpu/devices/cpu<id>/cache/index<index>/size.
> 
> Take care to do bounds checking when using fscanf() to read the
> contents of files into a string buffer because by default fscanf() assumes
> arbitrarily long strings. If the file contains more bytes than the array
> can accommodate then an overflow will occur.
> 
> Provide a maximum field width to the conversion specifier to protect
> against array overflow. The maximum is one less than the array size because
> string input stores a terminating null byte that is not covered by the
> maximum field width.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> This makes the code robust against any changes in information read
> from sysfs. The existing sysfs content fit well into the arrays, thus
> this is not considered a bugfix.
> 
> Changes since V2:
> - New patch
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 4 ++--
>  tools/testing/selftests/resctrl/resctrlfs.c   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index e88d5ca30517..c9dd70ce3ea8 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -159,7 +159,7 @@ static int read_from_imc_dir(char *imc_dir, int count)
>  
>  		return -1;
>  	}
> -	if (fscanf(fp, "%s", cas_count_cfg) <= 0) {
> +	if (fscanf(fp, "%1023s", cas_count_cfg) <= 0) {
>  		ksft_perror("Could not get iMC cas count read");
>  		fclose(fp);
>  
> @@ -177,7 +177,7 @@ static int read_from_imc_dir(char *imc_dir, int count)
>  
>  		return -1;
>  	}
> -	if  (fscanf(fp, "%s", cas_count_cfg) <= 0) {
> +	if  (fscanf(fp, "%1023s", cas_count_cfg) <= 0) {
>  		ksft_perror("Could not get iMC cas count write");
>  		fclose(fp);
>  
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 250c320349a7..a53cd1cb6e0c 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -182,7 +182,7 @@ int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size
>  
>  		return -1;
>  	}
> -	if (fscanf(fp, "%s", cache_str) <= 0) {
> +	if (fscanf(fp, "%63s", cache_str) <= 0) {
>  		ksft_perror("Could not get cache_size");
>  		fclose(fp);
>  
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.