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 -----