From: Andreas Reichel <[email protected]> * Fix missing format strings to avoid exploits or stack corruption * Fix warnings: * missing type casts * missing return values * ignored return values * possibly unitialized variables * unused variables * unintended integer overflows * dereference of freed pointers * Fix file offset size to 64 bits in Makefile
Signed-off-by: Andreas Reichel <[email protected]> --- Makefile.am | 3 ++- env/env_api.c | 3 +-- env/env_api_fat.c | 16 +++++++-------- env/uservars.c | 6 +++--- tools/bg_setenv.c | 59 ++++++++++++++++++++++++++++++++----------------------- tools/ebgpart.c | 55 ++++++++++++++++++++++++++++----------------------- 6 files changed, 78 insertions(+), 64 deletions(-) diff --git a/Makefile.am b/Makefile.am index c7dcb10..6618083 100644 --- a/Makefile.am +++ b/Makefile.am @@ -27,7 +27,8 @@ AM_CFLAGS = \ -Wmissing-prototypes \ -fshort-wchar \ -DHAVE_ENDIAN_H \ - -D_GNU_SOURCE + -D_GNU_SOURCE \ + -D_FILE_OFFSET_BITS=64 AM_LDFLAGS = -L$(top_builddir)/ diff --git a/env/env_api.c b/env/env_api.c index b3377fc..f6d3327 100644 --- a/env/env_api.c +++ b/env/env_api.c @@ -137,7 +137,6 @@ uint16_t ebg_env_getglobalstate(ebgenv_t *e) * with ustate == USTATE_FAILED */ if (env->data->revision == REVISION_FAILED && env->data->ustate == USTATE_FAILED) { - (void)bgenv_close(env); res = 3; } (void)bgenv_close(env); @@ -166,7 +165,7 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate) if (ustate > USTATE_FAILED) { return EINVAL; } - snprintf(buffer, sizeof(buffer), "%d", ustate); + (void)snprintf(buffer, sizeof(buffer), "%d", ustate); res = bgenv_set((BGENV *)e->bgenv, "ustate", "String", buffer, strlen(buffer)); diff --git a/env/env_api_fat.c b/env/env_api_fat.c index a16db61..354d7f8 100644 --- a/env/env_api_fat.c +++ b/env/env_api_fat.c @@ -78,7 +78,7 @@ bool mount_partition(CONFIG_PART *cfgpart) { char tmpdir_template[256]; char *mountpoint; - snprintf(tmpdir_template, 256, "%s", tmp_mnt_dir); + (void)snprintf(tmpdir_template, 256, "%s", tmp_mnt_dir); if (!cfgpart) { return false; } @@ -204,7 +204,7 @@ bool probe_config_partitions(CONFIG_PART *cfgpart) ped_device_probe_all(); - while (dev = ped_device_get_next(dev)) { + while ((dev = ped_device_get_next(dev))) { printf_debug("Device: %s\n", dev->model); PedDisk *pd = ped_disk_new(dev); if (!pd) { @@ -220,11 +220,11 @@ bool probe_config_partitions(CONFIG_PART *cfgpart) continue; } if (strncmp("/dev/mmcblk", dev->path, 11) == 0) { - snprintf(devpath, 4096, "%sp%u", dev->path, - part->num); + (void)snprintf(devpath, 4096, "%sp%u", + dev->path, part->num); } else { - snprintf(devpath, 4096, "%s%u", dev->path, - part->num); + (void)snprintf(devpath, 4096, "%s%u", + dev->path, part->num); } if (!cfgpart[count].devpath) { cfgpart[count].devpath = @@ -493,7 +493,7 @@ int bgenv_get(BGENV *env, char *key, char *type, void *data, uint32_t maxlen) } break; case EBGENV_WATCHDOG_TIMEOUT_SEC: - sprintf(buffer, "%lu", env->data->watchdog_timeout_sec); + sprintf(buffer, "%u", env->data->watchdog_timeout_sec); if (!data) { return strlen(buffer); } @@ -503,7 +503,7 @@ int bgenv_get(BGENV *env, char *key, char *type, void *data, uint32_t maxlen) } break; case EBGENV_REVISION: - sprintf(buffer, "%lu", env->data->revision); + sprintf(buffer, "%u", env->data->revision); if (!data) { return strlen(buffer); } diff --git a/env/uservars.c b/env/uservars.c index ffdd604..3359ec6 100644 --- a/env/uservars.c +++ b/env/uservars.c @@ -69,7 +69,7 @@ void bgenv_serialize_uservar(uint8_t *p, char *key, char *type, void *data, uint32_t payload_size, data_size; /* store key */ - strncpy(p, key, strlen(key) + 1); + strncpy((char *)p, key, strlen(key) + 1); p += strlen(key) + 1; /* store payload_size after key */ @@ -180,8 +180,8 @@ uint8_t *bgenv_uservar_alloc(uint8_t *udata, uint32_t datalen) return NULL; } spaceleft = bgenv_user_free(udata); - VERBOSE(stdout, "uservar_alloc: free: %u requested: %u \n", spaceleft, - datalen); + VERBOSE(stdout, "uservar_alloc: free: %lu requested: %lu \n", + (unsigned long)spaceleft, (unsigned long)datalen); /* To find the end of user variables, a 2nd 0 must be there after the * last variable content, thus, we need one extra byte if appending a diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c index 6f7f8f9..6b554c4 100644 --- a/tools/bg_setenv.c +++ b/tools/bg_setenv.c @@ -94,6 +94,7 @@ static void journal_process_action(BGENV *env, struct env_action *action) uint8_t *var; ebgenv_t e; uint16_t ustate; + unsigned long t; char *arg, *tmp; int ret; @@ -105,22 +106,24 @@ static void journal_process_action(BGENV *env, struct env_action *action) e.bgenv = env; arg = (char *)action->data; errno = 0; - ustate = strtol(arg, &tmp, 10); - if ((errno == ERANGE && (ustate == LONG_MAX || - ustate == LONG_MIN)) || - (errno != 0 && ustate == 0) || (tmp == arg)) { + t = strtol(arg, &tmp, 10); + if ((errno == ERANGE && (t == LONG_MAX || + t == LONG_MIN)) || + (errno != 0 && t == 0) || (tmp == arg)) { fprintf(stderr, "Invalid value for ustate: %s", (char *)action->data); return; } - if (ret = ebg_env_setglobalstate(&e, ustate) != 0) { - fprintf(stderr, "Error setting global state.", + ustate = (uint16_t)t;; + if ((ret = ebg_env_setglobalstate(&e, ustate)) != 0) { + fprintf(stderr, + "Error setting global state: %s.", strerror(ret)); } return; } bgenv_set(env, action->key, action->type, action->data, - strlen(action->data) + 1); + strlen((char *)action->data) + 1); break; case ENV_TASK_DEL: VERBOSE(stdout, "Task = DEL, key = %s\n", action->key); @@ -162,9 +165,10 @@ static uint8_t str2ustate(char *str) static char *ustate2str(uint8_t ustate) { - if (ustate >= USTATE_MIN && ustate <= USTATE_MAX) { - return ustatemap[ustate]; + if (ustate > USTATE_MAX) { + ustate = USTATE_MAX; } + return ustatemap[USTATE_MAX]; } static int set_uservars(char *arg) @@ -182,8 +186,8 @@ static int set_uservars(char *arg) return 0; } - journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT, value, - strlen(value) + 1); + journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT, + (uint8_t *)value, strlen(value) + 1); return 0; } @@ -192,7 +196,6 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) { struct arguments *arguments = state->input; int i; - wchar_t buffer[ENV_STRING_LENGTH]; char *tmp; switch (key) { @@ -204,8 +207,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) ENV_STRING_LENGTH); return 1; } - journal_add_action(ENV_TASK_SET, "kernelfile", "String", arg, - strlen(arg) + 1); + journal_add_action(ENV_TASK_SET, "kernelfile", "String", + (uint8_t *)arg, strlen(arg) + 1); break; case 'a': if (strlen(arg) > ENV_STRING_LENGTH) { @@ -215,8 +218,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) ENV_STRING_LENGTH); return 1; } - journal_add_action(ENV_TASK_SET, "kernelparams", "String", arg, - strlen(arg) + 1); + journal_add_action(ENV_TASK_SET, "kernelparams", "String", + (uint8_t *)arg, strlen(arg) + 1); break; case 'p': i = strtol(arg, &tmp, 10); @@ -258,7 +261,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) } else { asprintf(&tmp, "%u", i); journal_add_action(ENV_TASK_SET, "ustate", "String", - tmp, strlen(tmp) + 1); + (uint8_t *)tmp, strlen(tmp) + 1); VERBOSE(stdout, "Ustate set to %u (%s).\n", i, ustate2str(i)); } @@ -266,8 +269,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) case 'r': i = atoi(arg); VERBOSE(stdout, "Revision is set to %d.\n", i); - journal_add_action(ENV_TASK_SET, "revision", "String", arg, - strlen(arg) + 1); + journal_add_action(ENV_TASK_SET, "revision", "String", + (uint8_t *)arg, strlen(arg) + 1); break; case 'w': i = atoi(arg); @@ -275,7 +278,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) VERBOSE(stdout, "Setting watchdog timeout to %d seconds.\n", i); journal_add_action(ENV_TASK_SET, "watchdog_timeout_sec", - "String", arg, strlen(arg) + 1); + "String", (uint8_t *)arg, + strlen(arg) + 1); } else { fprintf(stderr, "Watchdog timeout must be non-zero.\n"); return 1; @@ -289,7 +293,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) VERBOSE(stdout, "Confirming environment to work. Removing boot-once " "and testing flag.\n"); - journal_add_action(ENV_TASK_SET, "ustate", "String", "0", 2); + journal_add_action(ENV_TASK_SET, "ustate", "String", + (uint8_t *)"0", 2); break; case 'u': if (part_specified) { @@ -367,11 +372,16 @@ static void update_environment(BGENV *env) journal_process_action(env, action); journal_free_action(action); STAILQ_REMOVE(&head, action, env_action, journal); - free(action); } env->data->crc32 = crc32(0, (const Bytef *)env->data, sizeof(BG_ENVDATA) - sizeof(env->data->crc32)); + + while (!STAILQ_EMPTY(&head)) { + action = STAILQ_FIRST(&head); + STAILQ_REMOVE_HEAD(&head, journal); + free(action); + } } static void dump_envs(void) @@ -418,7 +428,7 @@ int main(int argc, char **argv) STAILQ_INIT(&head); error_t e; - if (e = argp_parse(argp, argc, argv, 0, 0, &arguments)) { + if ((e = argp_parse(argp, argc, argv, 0, 0, &arguments))) { return e; } @@ -450,7 +460,7 @@ int main(int argc, char **argv) } if (fclose(of)) { fprintf(stderr, "Error closing output file.\n"); - result = 1; + result = errno; }; printf("Output written to %s.\n", envfilepath); } else { @@ -477,7 +487,6 @@ int main(int argc, char **argv) BGENV *env_new; BGENV *env_current; - char *tmp; if (auto_update) { /* clone latest environment */ diff --git a/tools/ebgpart.c b/tools/ebgpart.c index d0997bd..992099a 100644 --- a/tools/ebgpart.c +++ b/tools/ebgpart.c @@ -43,8 +43,9 @@ static void add_block_dev(PedDevice *dev) static char *GUID_to_str(uint8_t *g) { - snprintf(buffer, 37, "%02X%02X%02X%02X-%02X%02X-%02X%02X-%02X%02X-%02X%" - "02X%02X%02X%02X%02X", + (void)snprintf(buffer, 37, + "%02X%02X%02X%02X-%02X%02X-%02X%02X-%02X%02X-" + "%02X%02X%02X%02X%02X%02X", g[3], g[2], g[1], g[0], g[5], g[4], g[7], g[6], g[8], g[9], g[10], g[11], g[12], g[13], g[14], g[15]); return buffer; @@ -75,7 +76,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e, if (strcmp(GPT_PARTITION_GUID_FAT_NTFS, GUID_to_str(e->type_GUID)) != 0 && strcmp(GPT_PARTITION_GUID_ESP, GUID_to_str(e->type_GUID)) != 0) { - asprintf(&pfst->name, "not supported"); + (void)asprintf(&pfst->name, "%s", "not supported"); return true; } VERBOSE(stdout, "GPT Partition #%u is FAT/NTFS.\n", i); @@ -87,7 +88,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e, return false; } /* Look if it is a FAT12 or FAT16 */ - off64_t dest = e->start_LBA * LB_SIZE + 0x36; + off64_t dest = (off64_t)e->start_LBA * LB_SIZE + 0x36; if (lseek64(fd, dest, SEEK_SET) == -1) { VERBOSE(stderr, "Error seeking FAT12/16 Id String: %s\n", strerror(errno)); @@ -103,7 +104,7 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e, if (strcmp(FAT_id, "FAT12 ") != 0 && strcmp(FAT_id, "FAT16 ") != 0) { /* No FAT12/16 so read ID field for FAT32 */ - dest = e->start_LBA * LB_SIZE + 0x52; + dest = (off64_t)e->start_LBA * LB_SIZE + 0x52; if (lseek64(fd, dest, SEEK_SET) == -1) { VERBOSE(stderr, "Error seeking FAT32 Id String: %s\n", strerror(errno)); @@ -116,11 +117,11 @@ static bool check_GPT_FAT_entry(int fd, struct EFIpartitionentry *e, } } if (strcmp(FAT_id, "FAT12 ") == 0) { - asprintf(&pfst->name, "fat12"); + (void)asprintf(&pfst->name, "%s", "fat12"); } else if (strcmp(FAT_id, "FAT16 ") == 0) { - asprintf(&pfst->name, "fat16"); + (void)asprintf(&pfst->name, "%s", "fat16"); } else { - asprintf(&pfst->name, "fat32"); + (void)asprintf(&pfst->name, "%s", "fat32"); } VERBOSE(stdout, "GPT Partition #%u is %s.\n", i, pfst->name); if (lseek64(fd, curr, SEEK_SET) == -1) { @@ -191,19 +192,19 @@ static void scanLogicalVolumes(int fd, off64_t extended_start_LBA, PedPartition *partition, int lognum) { struct Masterbootrecord next_ebr; - PedFileSystemType *pfst; + PedFileSystemType *pfst = NULL; off64_t offset = extended_start_LBA + ebr->parttable[i].start_LBA; if (extended_start_LBA == 0) { extended_start_LBA = offset; } - VERBOSE(stdout, "Seeking to LBA %ld\n", offset); + VERBOSE(stdout, "Seeking to LBA %llu\n", (unsigned long long)offset); off64_t res = lseek64(fd, offset * LB_SIZE, SEEK_SET); if (res == -1) { VERBOSE(stderr, "(%s)\n", strerror(errno)); return; } - VERBOSE(stdout, "Seek returned %ld\n", res); + VERBOSE(stdout, "Seek returned %lld\n", (signed long long)res); if (read(fd, &next_ebr, sizeof(next_ebr)) != sizeof(next_ebr)) { VERBOSE(stderr, "Error reading next EBR (%s)\n", strerror(errno)); @@ -233,7 +234,7 @@ static void scanLogicalVolumes(int fd, off64_t extended_start_LBA, if (!pfst) { goto scl_out_of_mem; } - if (asprintf(&pfst->name, type_to_name(t)) == -1) { + if (asprintf(&pfst->name, "%s", type_to_name(t)) == -1) { goto scl_out_of_mem; }; partition = partition->next; @@ -270,6 +271,7 @@ static bool check_partition_table(PedDevice *dev) } int numpartitions = 0; PedPartition **list_end = &dev->part_list; + PedPartition *tmp = NULL; for (int i = 0; i < 4; i++) { if (mbr.parttable[i].partition_type == 0) { continue; @@ -281,10 +283,12 @@ static bool check_partition_table(PedDevice *dev) if (t == MBR_TYPE_GPT) { VERBOSE(stdout, "GPT header at %X\n", mbr.parttable[i].start_LBA); - off64_t offset = LB_SIZE * mbr.parttable[i].start_LBA; + off64_t offset = LB_SIZE * + (off64_t)mbr.parttable[i].start_LBA; if (lseek64(fd, offset, SEEK_SET) != offset) { VERBOSE(stderr, "Error seeking EFI Header\n."); VERBOSE(stderr, "(%s)", strerror(errno)); + close(fd); return false; } struct EFIHeader efihdr; @@ -301,8 +305,8 @@ static bool check_partition_table(PedDevice *dev) efihdr.signature[6], efihdr.signature[7]); VERBOSE(stdout, "Number of partition entries: %u\n", efihdr.partitions); - VERBOSE(stdout, "Partition Table @ LBA %lu\n", - efihdr.partitiontable_LBA); + VERBOSE(stdout, "Partition Table @ LBA %llu\n", + (unsigned long long)efihdr.partitiontable_LBA); read_GPT_entries(fd, efihdr.partitiontable_LBA, efihdr.partitions, dev); break; @@ -312,7 +316,7 @@ static bool check_partition_table(PedDevice *dev) goto cpt_out_of_mem; } - PedPartition *tmp = calloc(sizeof(PedPartition), 1); + tmp = calloc(sizeof(PedPartition), 1); if (!tmp) { goto cpt_out_of_mem; } @@ -324,7 +328,7 @@ static bool check_partition_table(PedDevice *dev) list_end = &((*list_end)->next); if (t == MBR_TYPE_EXTENDED || t == MBR_TYPE_EXTENDED_LBA) { - asprintf(&pfst->name, "extended"); + (void)asprintf(&pfst->name, "%s", "extended"); scanLogicalVolumes(fd, 0, &mbr, i, tmp, 5); /* Could be we still have MBR entries after * logical volumes */ @@ -332,7 +336,7 @@ static bool check_partition_table(PedDevice *dev) list_end = &((*list_end)->next); } } else { - asprintf(&pfst->name, type_to_name(t)); + (void)asprintf(&pfst->name, "%s", type_to_name(t)); } continue; cpt_out_of_mem: @@ -363,7 +367,8 @@ static int scan_devdir(unsigned int fmajor, unsigned int fminor, char *fullname, if (!devfile) { break; } - snprintf(fullname, maxlen, "%s/%s", DEVDIR, devfile->d_name); + (void)snprintf(fullname, maxlen, "%s/%s", DEVDIR, + devfile->d_name); struct stat fstat; if (stat(fullname, &fstat) == -1) { VERBOSE(stderr, "stat failed on %s\n", fullname); @@ -389,7 +394,7 @@ static int get_major_minor(char *filename, unsigned int *major, unsigned int *mi return -1; } int res = fscanf(fh, "%u:%u", major, minor); - fclose(fh); + (void)fclose(fh); if (res < 2) { VERBOSE(stderr, "Error reading major/minor of device entry. (%s)\n", @@ -402,7 +407,7 @@ static int get_major_minor(char *filename, unsigned int *major, unsigned int *mi void ped_device_probe_all(void) { struct dirent *sysblockfile; - char fullname[DEV_FILENAME_LEN]; + char fullname[DEV_FILENAME_LEN+16]; DIR *sysblockdir = opendir(SYSBLOCKDIR); if (!sysblockdir) { @@ -420,7 +425,7 @@ void ped_device_probe_all(void) strcmp(sysblockfile->d_name, "..") == 0) { continue; } - snprintf(fullname, sizeof(fullname), "/sys/block/%s/dev", + (void)snprintf(fullname, sizeof(fullname), "/sys/block/%s/dev", sysblockfile->d_name); /* Get major and minor revision from /sys/block/sdX/dev */ unsigned int fmajor, fminor; @@ -431,7 +436,7 @@ void ped_device_probe_all(void) "Trying device with: Major = %d, Minor = %d, (%s)\n", fmajor, fminor, fullname); /* Check if this file is really in the dev directory */ - snprintf(fullname, sizeof(fullname), "%s/%s", DEVDIR, + (void)snprintf(fullname, sizeof(fullname), "%s/%s", DEVDIR, sysblockfile->d_name); struct stat fstat; if (stat(fullname, &fstat) == -1) { @@ -444,8 +449,8 @@ void ped_device_probe_all(void) } /* This is a block device, so add it to the list*/ PedDevice *dev = calloc(sizeof(PedDevice), 1); - asprintf(&dev->model, "N/A"); - asprintf(&dev->path, "%s", fullname); + (void)asprintf(&dev->model, "%s", "N/A"); + (void)asprintf(&dev->path, "%s", fullname); if (check_partition_table(dev)) { add_block_dev(dev); } else { -- 2.14.1 -- You received this message because you are subscribed to the Google Groups "EFI Boot Guard" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/20170928131646.19150-1-andreas.reichel.ext%40siemens.com. For more options, visit https://groups.google.com/d/optout.
