[scan-admin@coverity.com: New Defects reported by Coverity Scan for Das U-Boot]
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-admin@coverity.com -----
Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@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); 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)); 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); 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); 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.
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".
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); 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.
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.
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-2F2Czv4BOaCWWCy7my0P0...
To manage Coverity Scan email notifications for "tom.rini@gmail.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
----- End forwarded message -----
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-admin@coverity.com -----
Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@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); 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)); 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); 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); 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.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".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); 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.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.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-2F2Czv4BOaCWWCy7my0P0...
To manage Coverity Scan email notifications for "tom.rini@gmail.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
----- End forwarded message -----
Dear Tom,
thanks for providing the Coverity results.
I hope Sugosh and Takahiro will take care of the EFI capsule stuff.
Find my comments for some other findings below.
On 1/20/21 9:43 PM, 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-admin@coverity.com -----
Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@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); 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()
I will create a patch to remove the unnecessary check.
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()
I will add the missing return value handling.
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)); 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); 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); 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 }
We are writing here adjacent fields shortname (8 chars) and extension (3 chars). This saves a second memcpy() call for the adjacent field.
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.
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".
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); 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.
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);
We are copying to two adjacent fields (filename and extension) which together have 11 bytes. This saves a second memcpy call.
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.
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);
We are copying to two adjacent fields (filename and extension) which together have 11 bytes. This saves a second memcpy call.
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".
Yes, we want to cause a segmentation fault here to test the crash handler.
Best regards
Heinrich
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-2F2Czv4BOaCWWCy7my0P0...
To manage Coverity Scan email notifications for "tom.rini@gmail.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
----- End forwarded message -----
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-admin@coverity.com -----
Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@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-2F2Czv4BOaCWWCy7my0P0...
To manage Coverity Scan email notifications for "tom.rini@gmail.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
----- End forwarded message -----
On Thu, Jan 21, 2021 at 11:09:16AM +0900, AKASHI Takahiro wrote:
Tom,
Regarding EFI capsule update,
[snip]
** 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?
I've updated Coverity to list both of these as intentional / ignore, thanks.
On Wed, 20 Jan 2021 14:04:18 -0500 Tom Rini trini@konsulko.com wrote:
Hi Tom,
I decided to run Coverity part-way through the merge window this time and here's what's been found so far.
Thanks for that!
----- Forwarded message from scan-admin@coverity.com -----
Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@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".
So this is a false report, as string_pool is 13 *words*: uint32_t string_pool[13]; And I explicitly used sizeof() to avoid any ambiguities here.
One could argue that this is at least misleading for a human reader, and a string pool should indeed be made of "char"s (which looks like indeed worth a patch), but the buffer is definitely 52 bytes long (and sizeof reports that). Not sure if that's worth reporting to Coverity, or we do just ignore it?
Cheers, Andre
96 strcpy((char *)header->string_pool, params->imagename);
On Wed, Jan 20, 2021 at 09:03:40PM +0000, Andre Przywara wrote:
On Wed, 20 Jan 2021 14:04:18 -0500 Tom Rini trini@konsulko.com wrote:
Hi Tom,
I decided to run Coverity part-way through the merge window this time and here's what's been found so far.
Thanks for that!
----- Forwarded message from scan-admin@coverity.com -----
Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@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".
So this is a false report, as string_pool is 13 *words*: uint32_t string_pool[13]; And I explicitly used sizeof() to avoid any ambiguities here.
One could argue that this is at least misleading for a human reader, and a string pool should indeed be made of "char"s (which looks like indeed worth a patch), but the buffer is definitely 52 bytes long (and sizeof reports that). Not sure if that's worth reporting to Coverity, or we do just ignore it?
I'll mark it as false positive with your explanation above, thanks!
On Thu, 21 Jan 2021 at 00:34, Tom Rini trini@konsulko.com wrote:
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-admin@coverity.com -----
Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com To: tom.rini@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 316356: Resource leaks (RESOURCE_LEAK) /tools/mkeficapsule.c: 225 in add_public_key()
<snip>
*** 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.225 return 0; 226 227 err: 228 if (sptr) 229 munmap(sptr, src_size); 230
I think these should not cause any issues, since the function return results in the process termination in both the scenarios of success and failure. But i will post a patch to handle these errors to keep the resource handling consistent.
-sughosh
On 21.01.21 12:36, Sughosh Ganu wrote:
On Thu, 21 Jan 2021 at 00:34, Tom Rini <trini@konsulko.com mailto:trini@konsulko.com> wrote:
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-admin@coverity.com <mailto:scan-admin@coverity.com> ----- Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com <mailto:scan-admin@coverity.com> To: tom.rini@gmail.com <mailto:tom.rini@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 316356: Resource leaks (RESOURCE_LEAK) /tools/mkeficapsule.c: 225 in add_public_key()<snip>
________________________________________________________________________________________________________ *** 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. 225 return 0; 226 227 err: 228 if (sptr) 229 munmap(sptr, src_size); 230I think these should not cause any issues, since the function return results in the process termination in both the scenarios of success and failure. But i will post a patch to handle these errors to keep the resource handling consistent.
Looking at line 234f:
if (srcfd >= 0) close(srcfd);
The comparison is wrong. It should be:
if (srcfd != -1) close(srcfd);
The open.2 man-page says that only -1 signals an error. According to the man-page -2 is a legal value for a file descriptor.
The initialization of destfd is wrong:
141: int destfd = 0;
In case of an error opening srcfd this leads to closing file descriptor 0 which relates to the console input. You should use:
int destfd = -1;
and
if (destfd != -1) close(destfd);
Best regards
Heinrich
On Thu, 21 Jan 2021 at 19:14, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 21.01.21 12:36, Sughosh Ganu wrote:
On Thu, 21 Jan 2021 at 00:34, Tom Rini <trini@konsulko.com mailto:trini@konsulko.com> wrote:
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-admin@coverity.com <mailto:scan-admin@coverity.com> ----- Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com <mailto:scan-admin@coverity.com> To: tom.rini@gmail.com <mailto:tom.rini@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 316356: Resource leaks (RESOURCE_LEAK) /tools/mkeficapsule.c: 225 in add_public_key()<snip>
*** 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. 225 return 0; 226 227 err: 228 if (sptr) 229 munmap(sptr, src_size); 230I think these should not cause any issues, since the function return results in the process termination in both the scenarios of success and failure. But i will post a patch to handle these errors to keep the resource handling consistent.
Looking at line 234f:
if (srcfd >= 0) close(srcfd);The comparison is wrong. It should be:
if (srcfd != -1) close(srcfd);The open.2 man-page says that only -1 signals an error. According to the man-page -2 is a legal value for a file descriptor.
Can you point me to which man page you are referring to. The open manpage on my ubuntu system has the following,
"The return value of open() is a file descriptor, a small, nonnegative integer that is used in subsequent system calls"
I could not find any mention of -2 being a valid file descriptor.
-sughosh
Am 22. Januar 2021 09:54:20 MEZ schrieb Sughosh Ganu sughosh.ganu@linaro.org:
On Thu, 21 Jan 2021 at 19:14, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 21.01.21 12:36, Sughosh Ganu wrote:
On Thu, 21 Jan 2021 at 00:34, Tom Rini <trini@konsulko.com mailto:trini@konsulko.com> wrote:
I decided to run Coverity part-way through the merge windowthis time
and here's what's been found so far. ----- Forwarded message from scan-admin@coverity.com <mailto:scan-admin@coverity.com> ----- Date: Mon, 18 Jan 2021 17:53:19 +0000 (UTC) From: scan-admin@coverity.com <mailto:scan-admin@coverity.com> To: tom.rini@gmail.com <mailto:tom.rini@gmail.com> Subject: New Defects reported by Coverity Scan for Das U-Boot Hi, Please find the latest report on new defect(s) introduced toDas
U-Boot found with Coverity Scan. 23 new defect(s) introduced to Das U-Boot found with CoverityScan.
2 defect(s), reported by Coverity Scan earlier, were markedfixed in
the recent build analyzed by Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 20 of 23 defect(s) ** CID 316356: Resource leaks (RESOURCE_LEAK) /tools/mkeficapsule.c: 225 in add_public_key()<snip>
*** CID 316356: Resource leaks (RESOURCE_LEAK) /tools/mkeficapsule.c: 225 in add_public_key() 219 if (ret < 0) { 220 fprintf(stderr, "%s: Unable to addpublic
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 thehandle.
225 return 0; 226 227 err: 228 if (sptr) 229 munmap(sptr, src_size); 230I think these should not cause any issues, since the function
return
results in the process termination in both the scenarios of success
and
failure. But i will post a patch to handle these errors to keep the resource handling consistent.
Looking at line 234f:
if (srcfd >= 0) close(srcfd);The comparison is wrong. It should be:
if (srcfd != -1) close(srcfd);The open.2 man-page says that only -1 signals an error. According to
the
man-page -2 is a legal value for a file descriptor.
Can you point me to which man page you are referring to. The open manpage on my ubuntu system has the following,
"The return value of open() is a file descriptor, a small, nonnegative integer that is used in subsequent system calls"
I could not find any mention of -2 being a valid file descriptor.
-sughosh
You are right
https://pubs.opengroup.org/onlinepubs/9699919799/
says the return value must be positive or -1 (in case of an error).
participants (5)
-
AKASHI Takahiro -
Andre Przywara -
Heinrich Schuchardt -
Sughosh Ganu -
Tom Rini