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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <70d4206c-3a5f-e699-0608-f70751f124eb@linux.intel.com>
Date: Fri, 18 Oct 2024 11:53:21 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Reinette Chatre <reinette.chatre@...el.com>
cc: fenghua.yu@...el.com, shuah@...nel.org, tony.luck@...el.com, 
    peternewman@...gle.com, babu.moger@....com, 
    Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>, 
    linux-kselftest@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 05/15] selftests/resctrl: Protect against array overflow
 when reading strings

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@...el.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@...ux.intel.com>

-- 
 i.