diff mbox series

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

Message ID 4adf01b3ee7019163ea4fc00b5d03d514d41b4b7.1729218182.git.reinette.chatre@intel.com (mailing list archive)
State Accepted
Commit 46058430fc5d39c114f7e1b9c6ff14c9f41bd531
Headers show
Series selftests/resctrl: Support diverse platforms with MBM and MBA tests | expand

Commit Message

Reinette Chatre Oct. 18, 2024, 2:33 a.m. UTC
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(-)

Comments

Ilpo Järvinen Oct. 18, 2024, 8:53 a.m. UTC | #1
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>
diff mbox series

Patch

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);