Tom, Regarding EFI capsule update,
On Wed, Jan 20, 2021 at 09:43:57PM +0100, Heinrich Schuchardt wrote: > On 1/20/21 8:04 PM, Tom Rini wrote: > > CC: Takahiro > > > I decided to run Coverity part-way through the merge window this time > > and here's what's been found so far. > > > > ----- Forwarded message from scan-ad...@coverity.com ----- > > > > Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) > > From: scan-ad...@coverity.com > > To: tom.r...@gmail.com > > Subject: New Defects reported by Coverity Scan for Das U-Boot > > > > Hi, > > > > Please find the latest report on new defect(s) introduced to Das U-Boot > > found with Coverity Scan. > > > > 23 new defect(s) introduced to Das U-Boot found with Coverity Scan. > > 2 defect(s), reported by Coverity Scan earlier, were marked fixed in the > > recent build analyzed by Coverity Scan. > > > > New defect(s) Reported-by: Coverity Scan > > Showing 20 of 23 defect(s) > > > > > > ** CID 316365: Memory - corruptions (STRING_OVERFLOW) > > /tools/sunxi_egon.c: 96 in egon_set_header() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316365: Memory - corruptions (STRING_OVERFLOW) > > /tools/sunxi_egon.c: 96 in egon_set_header() > > 90 > > 91 /* If an image name has been provided, use it as the DT name. */ > > 92 if (params->imagename && params->imagename[0]) { > > 93 if (strlen(params->imagename) > > > sizeof(header->string_pool) - 1) > > 94 printf("WARNING: DT name too long for SPL > > header!\n"); > > 95 else { > > > > > CID 316365: Memory - corruptions (STRING_OVERFLOW) > > > > > You might overrun the 13-character destination string > > > > > "header->string_pool" by writing 51 characters from > > > > > "params->imagename". > > 96 strcpy((char *)header->string_pool, > > params->imagename); > > 97 value = offsetof(struct boot_file_head, > > string_pool); > > 98 header->dt_name_offset = cpu_to_le32(value); > > 99 header->spl_signature[3] = > > SPL_DT_HEADER_VERSION; > > 100 } > > 101 } > > > > ** CID 316364: Null pointer dereferences (FORWARD_NULL) > > /cmd/efidebug.c: 202 in do_efi_capsule_res() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316364: Null pointer dereferences (FORWARD_NULL) > > /cmd/efidebug.c: 202 in do_efi_capsule_res() > > 196 printf("Failed to get %ls\n", var_name16); > > 197 > > 198 return CMD_RET_FAILURE; > > 199 } > > 200 } > > 201 > > > > > CID 316364: Null pointer dereferences (FORWARD_NULL) > > > > > Dereferencing null pointer "result". > > 202 printf("Result total size: 0x%x\n", > > result->variable_total_size); This is basically safe because a buffer for "result" is allocated by malloc(). (The second "get_variable" fails any way if the allocation has failed.) But there may be a chance (unlikely though) that the first "get_variable" will return neither EFI_SUCCESS or EFI_BUFFER_TOO_SMALL. I will modify the code a bit to address that. > > 203 printf("Capsule guid: %pUl\n", &result->capsule_guid); > > 204 printf("Time processed: %04d-%02d-%02d %02d:%02d:%02d\n", > > 205 result->capsule_processed.year, > > result->capsule_processed.month, > > 206 result->capsule_processed.day, > > result->capsule_processed.hour, > > 207 result->capsule_processed.minute, > > > > ** CID 316363: Null pointer dereferences (REVERSE_INULL) > > /lib/efi_loader/efi_boottime.c: 1993 in efi_load_image_from_path() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316363: Null pointer dereferences (REVERSE_INULL) > > /lib/efi_loader/efi_boottime.c: 1993 in efi_load_image_from_path() > > 1987 ret = EFI_CALL(load_file_protocol->load_file( > > 1988 load_file_protocol, dp, > > boot_policy, > > 1989 &buffer_size, (void > > *)(uintptr_t)addr)); > > 1990 if (ret != EFI_SUCCESS) > > 1991 efi_free_pages(addr, pages); > > 1992 out: > > > > > CID 316363: Null pointer dereferences (REVERSE_INULL) > > > > > Null-checking "load_file_protocol" suggests that it may be null, > > > > > but it has already been dereferenced on all paths leading to the > > > > > check. > > 1993 if (load_file_protocol) > > 1994 EFI_CALL(efi_close_protocol(device, > > 1995 > > &efi_guid_load_file2_protocol, > > 1996 efi_root, NULL)); > > 1997 if (ret == EFI_SUCCESS) { > > 1998 *buffer = (void *)(uintptr_t)addr; > > > > ** CID 316362: Error handling issues (CHECKED_RETURN) > > /fs/fat/fat_write.c: 422 in fill_dir_slot() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316362: Error handling issues (CHECKED_RETURN) > > /fs/fat/fat_write.c: 422 in fill_dir_slot() > > 416 while (counter >= 1) { > > 417 memcpy(itr->dent, slotptr, sizeof(dir_slot)); > > 418 slotptr--; > > 419 counter--; > > 420 > > 421 if (itr->remaining == 0) > > > > > CID 316362: Error handling issues (CHECKED_RETURN) > > > > > Calling "flush_dir" without checking return value (as is done > > > > > elsewhere 5 out of 6 times). > > 422 flush_dir(itr); > > 423 > > 424 next_dent(itr); > > 425 if (!itr->dent) > > 426 return -EIO; > > 427 } > > > > ** CID 316361: Code maintainability issues (SIZEOF_MISMATCH) > > /lib/efi_loader/efi_capsule.c: 767 in efi_capsule_scan_dir() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316361: Code maintainability issues (SIZEOF_MISMATCH) > > /lib/efi_loader/efi_capsule.c: 767 in efi_capsule_scan_dir() > > 761 > > 762 ret = EFI_CALL((*dirh->setpos)(dirh, 0)); > > 763 if (ret != EFI_SUCCESS) > > 764 goto err; > > 765 > > 766 /* make a list */ > > > > > CID 316361: Code maintainability issues (SIZEOF_MISMATCH) > > > > > Passing argument "count * 8UL /* sizeof (*files) */" to function > > > > > "dlmalloc" and then casting the return value to "u16 **" is > > > > > suspicious. In this particular case "sizeof (u16 **)" happens to be > > > > > equal to "sizeof (u16 *)", but this is not a portable assumption. > > 767 tmp_files = malloc(count * sizeof(*files)); I will fix this by modifying the code to: tmp_files = malloc(count * sizeof(tmp_files[0])); > > 768 if (!tmp_files) { > > 769 ret = EFI_OUT_OF_RESOURCES; > > 770 goto err; > > 771 } > > 772 > > > > ** CID 316360: Uninitialized variables (UNINIT) > > /tools/mkeficapsule.c: 298 in create_fwbin() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316360: Uninitialized variables (UNINIT) > > /tools/mkeficapsule.c: 298 in create_fwbin() > > 292 goto err_3; > > 293 } > > 294 > > 295 capsule.version = 0x00000001; > > 296 capsule.embedded_driver_count = 0; > > 297 capsule.payload_item_count = 1; > > > > > CID 316360: Uninitialized variables (UNINIT) > > > > > Using uninitialized value "capsule". Field > > > > > "capsule.item_offset_list" is uninitialized when calling "fwrite". > > 298 size = fwrite(&capsule, 1, sizeof(capsule), f); This code is safe because capsule.item_offset_list is actually defined as "item_offset_list[]" (null array) at the end of the structure and the data will be filled in by the succeeding fwrite()'s. What action should be taken to suppress this warning? > > 299 if (size < (sizeof(capsule))) { > > 300 printf("write failed (%lx)\n", size); > > 301 goto err_3; > > 302 } > > 303 offset = sizeof(capsule) + sizeof(u64); > > > > ** CID 316359: Null pointer dereferences (FORWARD_NULL) > > > > > > ________________________________________________________________________________________________________ > > *** CID 316359: Null pointer dereferences (FORWARD_NULL) > > /lib/efi_loader/efi_capsule.c: 380 in efi_capsule_update_firmware() > > 374 ret = EFI_UNSUPPORTED; > > 375 goto out; > > 376 } > > 377 > > 378 /* find a device for update firmware */ > > 379 /* TODO: should we pass index as well, or nothing but > > type? */ > > > > > CID 316359: Null pointer dereferences (FORWARD_NULL) > > > > > Passing null pointer "handles" to "efi_fmp_find", which > > > > > dereferences it. > > 380 fmp = efi_fmp_find(&image->update_image_type_id, > > 381 image->update_hardware_instance, > > 382 handles, no_handles); This code is safe because "handles" is actually an array of pointers and "no_handles" indicates the number of elements in this array. efi_fmp_find() will not dereference handles at all if no_handles is zero. What action should be taken to suppress this warning? > > 383 if (!fmp) { > > 384 log_err("EFI Capsule: driver not found for > > firmware type: %pUl, hardware instance: %lld\n", > > 385 &image->update_image_type_id, > > > > ** CID 316358: Memory - illegal accesses (BUFFER_SIZE_WARNING) > > /drivers/net/sandbox-raw.c: 163 in sb_eth_raw_of_to_plat() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316358: Memory - illegal accesses (BUFFER_SIZE_WARNING) > > /drivers/net/sandbox-raw.c: 163 in sb_eth_raw_of_to_plat() > > 157 int ret; > > 158 > > 159 pdata->iobase = dev_read_addr(dev); > > 160 > > 161 ifname = dev_read_string(dev, "host-raw-interface"); > > 162 if (ifname) { > > > > > CID 316358: Memory - illegal accesses (BUFFER_SIZE_WARNING) > > > > > Calling "strncpy" with a maximum size argument of 16 bytes on > > > > > destination array "priv->host_ifname" of size 16 bytes might leave > > > > > the destination string unterminated. > > 163 strncpy(priv->host_ifname, ifname, IFNAMSIZ); > > 164 printf(": Using %s from DT\n", priv->host_ifname); > > 165 } > > 166 if (dev_read_u32(dev, "host-raw-interface-idx", > > 167 &priv->host_ifindex) < 0) { > > 168 priv->host_ifindex = 0; > > > > ** CID 316357: Memory - corruptions (BUFFER_SIZE) > > /fs/fat/fat_write.c: 1154 in fill_dentry() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316357: Memory - corruptions (BUFFER_SIZE) > > /fs/fat/fat_write.c: 1154 in fill_dentry() > > 1148 > > 1149 set_start_cluster(mydata, dentptr, start_cluster); > > 1150 dentptr->size = cpu_to_le32(size); > > 1151 > > 1152 dentptr->attr = attr; > > 1153 > > > > > CID 316357: Memory - corruptions (BUFFER_SIZE) > > > > > You might overrun the 8 byte destination string "dentptr->name" > > > > > by writing the maximum 11 bytes from "shortname". > > 1154 memcpy(dentptr->name, shortname, SHORT_NAME_SIZE); > > 1155 } > > 1156 > > 1157 /** > > 1158 * find_directory_entry() - find a directory entry by filename > > 1159 * > > > > ** CID 316356: Resource leaks (RESOURCE_LEAK) > > /tools/mkeficapsule.c: 225 in add_public_key() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316356: Resource leaks (RESOURCE_LEAK) > > /tools/mkeficapsule.c: 225 in add_public_key() > > 219 if (ret < 0) { > > 220 fprintf(stderr, "%s: Unable to add public key to the > > FDT\n", > > 221 __func__); > > 222 goto err; > > 223 } > > 224 > > > > > CID 316356: Resource leaks (RESOURCE_LEAK) > > > > > Handle variable "srcfd" going out of scope leaks the handle. I'd defer to Sughosh. > > 225 return 0; > > 226 > > 227 err: > > 228 if (sptr) > > 229 munmap(sptr, src_size); > > 230 > > > > ** CID 316355: Null pointer dereferences (FORWARD_NULL) > > /lib/efi_loader/efi_capsule.c: 848 in efi_capsule_read_file() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316355: Null pointer dereferences (FORWARD_NULL) > > /lib/efi_loader/efi_capsule.c: 848 in efi_capsule_read_file() > > 842 } > > 843 ret = EFI_CALL((*fh->getinfo)(fh, &efi_file_info_guid, > > 844 &size, file_info)); > > 845 } > > 846 if (ret != EFI_SUCCESS) > > 847 goto err; > > > > > CID 316355: Null pointer dereferences (FORWARD_NULL) > > > > > Dereferencing null pointer "file_info". Same as CID 316364 above. > > 848 size = file_info->file_size; > > 849 free(file_info); > > 850 buf = malloc(size); > > 851 if (!buf) { > > 852 ret = EFI_OUT_OF_RESOURCES; > > 853 goto err; > > > > ** CID 316354: Uninitialized variables (UNINIT) > > /tools/mkeficapsule.c: 318 in create_fwbin() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316354: Uninitialized variables (UNINIT) > > /tools/mkeficapsule.c: 318 in create_fwbin() > > 312 image.update_image_index = index; > > 313 image.update_image_size = bin_stat.st_size; > > 314 image.update_vendor_code_size = 0; /* none */ > > 315 image.update_hardware_instance = instance; > > 316 image.image_capsule_support = 0; > > 317 > > > > > CID 316354: Uninitialized variables (UNINIT) > > > > > Using uninitialized value "image". Field "image.reserved" is > > > > > uninitialized when calling "fwrite". > > 318 size = fwrite(&image, 1, sizeof(image), f); "reserved" is reserved, but I'd like to set them to zero for safety. > > 319 if (size < sizeof(image)) { > > 320 printf("write failed (%lx)\n", size); > > 321 goto err_3; > > 322 } > > 323 size = fread(data, 1, bin_stat.st_size, g); > > > > ** CID 316353: Resource leaks (RESOURCE_LEAK) > > /tools/mkeficapsule.c: 225 in add_public_key() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316353: Resource leaks (RESOURCE_LEAK) > > /tools/mkeficapsule.c: 225 in add_public_key() > > 219 if (ret < 0) { > > 220 fprintf(stderr, "%s: Unable to add public key to the > > FDT\n", > > 221 __func__); > > 222 goto err; > > 223 } > > 224 > > > > > CID 316353: Resource leaks (RESOURCE_LEAK) > > > > > Variable "sptr" going out of scope leaks the storage it points > > > > > to. Defer to Sughosh. > > 225 return 0; > > 226 > > 227 err: > > 228 if (sptr) > > 229 munmap(sptr, src_size); > > 230 > > > > ** CID 316352: Security best practices violations (STRING_OVERFLOW) > > /drivers/dfu/dfu.c: 490 in dfu_fill_entity() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316352: Security best practices violations (STRING_OVERFLOW) > > /drivers/dfu/dfu.c: 490 in dfu_fill_entity() > > 484 char *interface, char *devstr) > > 485 { > > 486 char *st; > > 487 > > 488 debug("%s: %s interface: %s dev: %s\n", __func__, s, interface, > > devstr); > > 489 st = strsep(&s, " "); > > > > > CID 316352: Security best practices violations > > > > > (STRING_OVERFLOW) > > > > > You might overrun the 32-character fixed-size string "dfu->name" > > > > > by copying "st" without checking the length. > > 490 strcpy(dfu->name, st); > > 491 > > 492 dfu->alt = alt; > > 493 dfu->max_buf_size = 0; > > 494 dfu->free_entity = NULL; > > 495 > > > > ** CID 316351: Error handling issues (CHECKED_RETURN) > > /drivers/video/pwm_backlight.c: 230 in pwm_backlight_of_to_plat() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316351: Error handling issues (CHECKED_RETURN) > > /drivers/video/pwm_backlight.c: 230 in pwm_backlight_of_to_plat() > > 224 cell = dev_read_prop(dev, "brightness-levels", &len); > > 225 count = len / sizeof(u32); > > 226 if (cell && count > index) { > > 227 priv->levels = malloc(len); > > 228 if (!priv->levels) > > 229 return log_ret(-ENOMEM); > > > > > CID 316351: Error handling issues (CHECKED_RETURN) > > > > > Calling "dev_read_u32_array" without checking return value (as > > > > > is done elsewhere 8 out of 9 times). > > 230 dev_read_u32_array(dev, "brightness-levels", > > priv->levels, > > 231 count); > > 232 priv->num_levels = count; > > 233 priv->default_level = priv->levels[index]; > > 234 priv->max_level = priv->levels[count - 1]; > > 235 } else { > > > > ** CID 316350: Memory - corruptions (OVERRUN) > > /fs/fat/fat_write.c: 1154 in fill_dentry() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316350: Memory - corruptions (OVERRUN) > > /fs/fat/fat_write.c: 1154 in fill_dentry() > > 1148 > > 1149 set_start_cluster(mydata, dentptr, start_cluster); > > 1150 dentptr->size = cpu_to_le32(size); > > 1151 > > 1152 dentptr->attr = attr; > > 1153 > > > > > CID 316350: Memory - corruptions (OVERRUN) > > > > > Overrunning array "dentptr->name" of 8 bytes by passing it to a > > > > > function which accesses it at byte offset 10 using argument "11UL". > > > > > [Note: The source code implementation of the function has been > > > > > overridden by a builtin model.] > > 1154 memcpy(dentptr->name, shortname, SHORT_NAME_SIZE); > > 1155 } > > 1156 > > 1157 /** > > 1158 * find_directory_entry() - find a directory entry by filename > > 1159 * > > > > ** CID 316349: Resource leaks (RESOURCE_LEAK) > > /tools/mkeficapsule.c: 225 in add_public_key() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316349: Resource leaks (RESOURCE_LEAK) > > /tools/mkeficapsule.c: 225 in add_public_key() > > 219 if (ret < 0) { > > 220 fprintf(stderr, "%s: Unable to add public key to the > > FDT\n", > > 221 __func__); > > 222 goto err; > > 223 } > > 224 > > > > > CID 316349: Resource leaks (RESOURCE_LEAK) > > > > > Handle variable "destfd" going out of scope leaks the handle. To Sughosh. -Takahiro Akashi > > 225 return 0; > > 226 > > 227 err: > > 228 if (sptr) > > 229 munmap(sptr, src_size); > > 230 > > > > ** CID 316348: Memory - corruptions (OVERRUN) > > /fs/fat/fat_write.c: 188 in set_name() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316348: Memory - corruptions (OVERRUN) > > /fs/fat/fat_write.c: 188 in set_name() > > 182 /* Each long name directory entry takes 13 characters. > > */ > > 183 ret = (strlen(filename) + 25) / 13; > > 184 goto out; > > 185 } > > 186 return -EIO; > > 187 out: > > > > > CID 316348: Memory - corruptions (OVERRUN) > > > > > Overrunning array "dirent.name" of 8 bytes by passing it to a > > > > > function which accesses it at byte offset 10 using argument "11UL". > > > > > [Note: The source code implementation of the function has been > > > > > overridden by a builtin model.] > > 188 memcpy(shortname, dirent.name, SHORT_NAME_SIZE); > > 189 return ret; > > 190 } > > 191 > > 192 static int total_sector; > > 193 static int disk_write(__u32 block, __u32 nr_blocks, void *buf) > > > > ** CID 316347: Null pointer dereferences (FORWARD_NULL) > > /cmd/sandbox/exception.c: 16 in do_sigsegv() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316347: Null pointer dereferences (FORWARD_NULL) > > /cmd/sandbox/exception.c: 16 in do_sigsegv() > > 10 > > 11 static int do_sigsegv(struct cmd_tbl *cmdtp, int flag, int argc, > > 12 char *const argv[]) > > 13 { > > 14 u8 *ptr = NULL; > > 15 > > > > > CID 316347: Null pointer dereferences (FORWARD_NULL) > > > > > Dereferencing null pointer "ptr". > > 16 *ptr = 0; > > 17 return CMD_RET_FAILURE; > > 18 } > > 19 > > 20 static int do_undefined(struct cmd_tbl *cmdtp, int flag, int argc, > > 21 char *const argv[]) > > > > ** CID 316346: Control flow issues (UNREACHABLE) > > /test/cmd/setexpr.c: 275 in setexpr_test_backref() > > > > > > ________________________________________________________________________________________________________ > > *** CID 316346: Control flow issues (UNREACHABLE) > > /test/cmd/setexpr.c: 275 in setexpr_test_backref() > > 269 "us \\1 \\2 \\3!", true)); > > 270 ut_asserteq_str("us this is surely! a test is it? yes us this > > is indeed! a test", > > 271 buf); > > 272 > > 273 /* The following checks fail at present due to a bug in setexpr > > */ > > 274 return 0; > > > > > CID 316346: Control flow issues (UNREACHABLE) > > > > > This code cannot be reached: "i = 256;". > > 275 for (i = BUF_SIZE; i < 0x1000; i++) { > > 276 ut_assertf(buf[i] == (char)i, > > 277 "buf byte at %x should be %02x, got %02x)\n", > > 278 i, i & 0xff, (u8)buf[i]); > > 279 ut_assertf(nbuf[i] == (char)i, > > 280 "nbuf byte at %x should be %02x, got > > %02x)\n", > > > > > > ________________________________________________________________________________________________________ > > To view the defects in Coverity Scan visit, > > https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yoA22WlOQ-2By3ieUvdbKmOyw68TMVT4Kip-2BBzfOGWXJ5yIiYplmPF9KAnKIja4Zd7tU-3DzXLV_EEm8SbLgSDsaDZif-2Bv7ch8WqhKpLoKErHi4nXpwDNTvNl0LKdSggNphGlGz-2FZpFlP-2B70lLxg94OYlINE3kVz2K7-2BaNONHtJP8TbjZRniVWbxuTUQjTtQl1N-2FQyFOjCv8gPw5EPU0ENb3p98VX92ve7SRBWt1r1v-2F-2F6AWroTa-2Bh7rN2QA2fbSgDcYmJ9RJ86TD6dhAH88KDOiq3Saai3zTbA9TSu9jcthFTuvEyi5KBE-3D > > > > To manage Coverity Scan email notifications for "tom.r...@gmail.com", > > click > > https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXxWeIHzDeopm-2BEWQ6S6K-2FtUHv9ZTk8qZbuzkkz9sa-2BJFw4elYDyedRVZOC-2ButxjBZdouVmTGuWB6Aj6G7lm7t25-2Biv1B-2B9082pHzCCex2kqMs-3DBleN_EEm8SbLgSDsaDZif-2Bv7ch8WqhKpLoKErHi4nXpwDNTvNl0LKdSggNphGlGz-2FZpFl83Kn4j1MsEeVR-2BhiT4TgLlRMzBzziPEpnjhf5UW-2FNLxwPg-2FlX4hM5uoZCyOPlCN-2BiReYf6wkiLt6iKknc3lnJUyqsWnyxIFGwSu2OUxAVy5vnsIFdRuglO4-2B9vJx2XrTM801x6AhuO0Zb5xr5hI9qgs9dwug2dbKvAt0T-2F-2Bv9VI-3D > > > > > > ----- End forwarded message ----- > > >