| Message ID | 20200407164928.915378-1-colin.king@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | acpi: battery: pass string length to avoid any string overflows | expand |
On 2020-04-07 10:49 a.m., Colin King wrote: > From: Colin Ian King <colin.king@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@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: Alex Hung <alex.hung@canonical.com>
On 4/8/20 12:49 AM, Colin King wrote: > From: Colin Ian King <colin.king@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@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@canonical.com>
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;