ACK: [PATCH] acpi: battery: pass string length to avoid any string overflows

[ivanhu]

On 4/8/20 12:49 AM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
> 
> Ensure we don't cause string overflows by passing the string
> length to the battery name fetching helper functions.  Also check
> if the battery name fetch occurred without error. Fixes a couple
> of issues found by static analysis with Coverity.
> 
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>  src/acpi/battery/battery.c     |  3 ++-
>  src/lib/include/fwts_battery.h |  2 +-
>  src/lib/src/fwts_battery.c     | 20 +++++++++++++-------
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/src/acpi/battery/battery.c b/src/acpi/battery/battery.c
> index 7444f5e8..e4cd5bd3 100644
> --- a/src/acpi/battery/battery.c
> +++ b/src/acpi/battery/battery.c
> @@ -256,7 +256,8 @@ static void do_battery_test(fwts_framework *fw, const uint32_t index)
>  
>  	*state = '\0';
>  
> -	fwts_battery_get_name(fw, index, name);
> +	if (fwts_battery_get_name(fw, index, name, sizeof(name)) != FWTS_OK)
> +		fwts_log_info(fw, "Cannot find batter name: cannot test.");
>  
>  	fwts_log_info(fw, "Test battery '%s'.", name);
>  
> diff --git a/src/lib/include/fwts_battery.h b/src/lib/include/fwts_battery.h
> index 800432f2..cb71db8d 100644
> --- a/src/lib/include/fwts_battery.h
> +++ b/src/lib/include/fwts_battery.h
> @@ -34,6 +34,6 @@ int fwts_battery_set_trip_point(fwts_framework *fw, const uint32_t index, const
>  int fwts_battery_get_trip_point(fwts_framework *fw, const uint32_t index, uint32_t *trip_point);
>  int fwts_battery_get_capacity(fwts_framework *fw, const fwts_battery_type type,
>  	const uint32_t index, uint32_t *capacity_mAh, uint32_t *capacity_mWh);
> -int fwts_battery_get_name(fwts_framework *fw, const uint32_t index, char *name);
> +int fwts_battery_get_name(fwts_framework *fw, const uint32_t index, char *name, const size_t name_len);
>  
>  #endif
> diff --git a/src/lib/src/fwts_battery.c b/src/lib/src/fwts_battery.c
> index 26fa0dab..3005b533 100644
> --- a/src/lib/src/fwts_battery.c
> +++ b/src/lib/src/fwts_battery.c
> @@ -25,6 +25,7 @@
>  #include <sys/stat.h>
>  #include <unistd.h>
>  #include <string.h>
> +#include <bsd/string.h>
>  #include <limits.h>
>  #include <dirent.h>
>  #include <inttypes.h>
> @@ -221,7 +222,8 @@ static int fwts_battery_get_count_proc_fs(DIR *dir, uint32_t *count)
>  static int fwts_battery_get_name_sys_fs(
>  	DIR *dir,
>  	const uint32_t index,
> -	char *name)
> +	char *name,
> +	const size_t name_len)
>  {
>  	struct dirent *entry;
>  	char path[PATH_MAX];
> @@ -247,7 +249,7 @@ static int fwts_battery_get_name_sys_fs(
>  			if (!match)
>  				continue;
>  
> -			strcpy(name, entry->d_name);
> +			strlcpy(name, entry->d_name, name_len);
>  			return FWTS_OK;
>  		}
>  	} while (entry);
> @@ -258,7 +260,8 @@ static int fwts_battery_get_name_sys_fs(
>  static int fwts_battery_get_name_proc_fs(
>  	DIR *dir,
>  	const uint32_t index,
> -	char *name)
> +	char *name,
> +	const size_t name_len)
>  {
>  	struct dirent *entry;
>  	uint32_t i = 0;
> @@ -271,7 +274,7 @@ static int fwts_battery_get_name_proc_fs(
>  			if (!match)
>  				continue;
>  
> -			strcpy(name, entry->d_name);
> +			strlcpy(name, entry->d_name, name_len);
>  			return FWTS_OK;
>  		}
>  	} while (entry);
> @@ -639,18 +642,21 @@ int fwts_battery_get_cycle_count(
>  int fwts_battery_get_name(
>  	fwts_framework *fw,
>  	const uint32_t index,
> -	char *name)
> +	char *name,
> +	const size_t name_len)
>  {
>  	int ret;
>  	DIR *dir;
>  
>  	FWTS_UNUSED(fw);
>  
> +	(void)memset(name, 0, name_len);
> +
>  	if ((dir = opendir(FWTS_SYS_CLASS_POWER_SUPPLY)) != NULL) {
> -		ret = fwts_battery_get_name_sys_fs(dir, index, name);
> +		ret = fwts_battery_get_name_sys_fs(dir, index, name, name_len);
>  		(void)closedir(dir);
>  	} else if ((dir = opendir(FWTS_PROC_ACPI_BATTERY)) != NULL) {
> -		ret = fwts_battery_get_name_proc_fs(dir, index, name);
> +		ret = fwts_battery_get_name_proc_fs(dir, index, name, name_len);
>  		(void)closedir(dir);
>  	} else {
>  		return FWTS_ERROR;
> 


Acked-by: Ivan Hu <ivan.hu at canonical.com>