On mer., juil. 24, 2024 at 11:21, Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
Hi Tom,
Thank you for the report.
On mar., juil. 23, 2024 at 08:18, Tom Rini trini@konsulko.com wrote:
Here's the latest report.
---------- Forwarded message --------- From: scan-admin@coverity.com Date: Mon, Jul 22, 2024, 8:07 PM Subject: New Defects reported by Coverity Scan for Das U-Boot To: tom.rini@gmail.com
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot found with Coverity Scan.
8 new defect(s) introduced to Das U-Boot found with Coverity Scan. 3 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 8 of 8 defect(s)
** CID 501795: Insecure data handling (TAINTED_SCALAR)
*** CID 501795: Insecure data handling (TAINTED_SCALAR) /boot/bootmeth_android.c: 96 in scan_boot_part() 90 if (!is_android_boot_image_header(buf)) { 91 free(buf); 92 return log_msg_ret("header", -ENOENT); 93 } 94 95 priv->header_version = ((struct andr_boot_img_hdr_v0 *)buf)->header_version;
CID 501795: Insecure data handling (TAINTED_SCALAR) Passing tainted expression "*buf" to "dlfree", which uses it as anoffset.
scan_boot_part() generates this warning, but scan_vendor_boot_part() does not. Both functions follow a similar code flow.
The only reason scan_boot_part() generates this warning, is because of the downcast into struct andr_boot_img_hdr_v0.
We can't change char* buf into struct andr_boot_img_hdr_v0 because we need to be block aligned when calling blk_dread().
Per my understanding tainted data means it comes from user input (which is true for both scan_boot_part() and scan_vendor_boot_part() because both read from eMMC, which can be consider "user input".
Since I don't see any particular problem with this code I propose that we ignore this warning.
96 free(buf); 97 98 return 0; 99 } 100 101 static int scan_vendor_boot_part(struct udevice *blk, struct android_priv *priv)
** CID 501794: Memory - corruptions (OVERRUN)
*** CID 501794: Memory - corruptions (OVERRUN) /lib/tpm_tcg2.c: 640 in tcg2_measurement_init() 634 rc = tcg2_log_prepare_buffer(*dev, elog, ignore_existing_log); 635 if (rc) { 636 tcg2_measurement_term(*dev, elog, true); 637 return rc; 638 } 639
CID 501794: Memory - corruptions (OVERRUN) Overrunning array "version_string" of 50 bytes by passing it to afunction which accesses it at byte offset 63. 640 rc = tcg2_measure_event(*dev, elog, 0, EV_S_CRTM_VERSION, 641 strlen(version_string) + 1, 642 (u8 *)version_string); 643 if (rc) { 644 tcg2_measurement_term(*dev, elog, true); 645 return rc;
** CID 501793: Insecure data handling (TAINTED_SCALAR) /lib/tpm-v2.c: 909 in tpm2_allow_extend()
*** CID 501793: Insecure data handling (TAINTED_SCALAR) /lib/tpm-v2.c: 909 in tpm2_allow_extend() 903 int rc; 904 905 rc = tpm2_get_pcr_info(dev, &pcrs); 906 if (rc) 907 return false; 908
CID 501793: Insecure data handling (TAINTED_SCALAR) Using tainted variable "pcrs.count" as a loop boundary.909 for (i = 0; i < pcrs.count; i++) { 910 if (tpm2_is_active_pcr(&pcrs.selection[i]) && 911 !tpm2_algorithm_to_len(pcrs.selection[i].hash)) 912 return false; 913 } 914 915 return true;
** CID 501792: Control flow issues (DEADCODE) /lib/efi_loader/efi_helper.c: 137 in efi_load_option_dp_join()
*** CID 501792: Control flow issues (DEADCODE) /lib/efi_loader/efi_helper.c: 137 in efi_load_option_dp_join() 131 if (fdt_dp) { 132 struct efi_device_path *tmp_dp = *dp; 133 134 *dp = efi_dp_concat(tmp_dp, fdt_dp, *dp_size); 135 efi_free_pool(tmp_dp); 136 if (!dp)
CID 501792: Control flow issues (DEADCODE) Execution cannot reach this statement: "return9223372036854775817UL;". 137 return EFI_OUT_OF_RESOURCES; 138 *dp_size += efi_dp_size(fdt_dp) + sizeof(END); 139 } 140 141 *dp_size += sizeof(END); 142
** CID 501791: (DEADCODE) /drivers/usb/gadget/ether.c: 2219 in eth_bind() /drivers/usb/gadget/ether.c: 2110 in eth_bind() /drivers/usb/gadget/ether.c: 2071 in eth_bind() /drivers/usb/gadget/ether.c: 2089 in eth_bind()
*** CID 501791: (DEADCODE) /drivers/usb/gadget/ether.c: 2219 in eth_bind() 2213 out_ep->name, in_ep->name, 2214 status_ep ? " STATUS " : "", 2215 status_ep ? status_ep->name : "" 2216 ); 2217 printf("MAC %pM\n", pdata->enetaddr); 2218
CID 501791: (DEADCODE) Execution cannot reach the expression "rndis" inside thisstatement: "if (cdc || rndis) printf(...". 2219 if (cdc || rndis) 2220 printf("HOST MAC %02x:%02x:%02x:%02x:%02x:%02x\n", 2221 dev->host_mac[0], dev->host_mac[1], 2222 dev->host_mac[2], dev->host_mac[3], 2223 dev->host_mac[4], dev->host_mac[5]); 2224 /drivers/usb/gadget/ether.c: 2110 in eth_bind() 2104 device_desc.bNumConfigurations = 2; 2105 2106 if (gadget_is_dualspeed(gadget)) { 2107 if (rndis) 2108 dev_qualifier.bNumConfigurations = 2; 2109 else if (!cdc)
CID 501791: (DEADCODE) Execution cannot reach this statement: "dev_qualifier.bDeviceClass...". 2110 dev_qualifier.bDeviceClass = USB_CLASS_VENDOR_SPEC; 2111 2112 /* assumes ep0 uses the same value for both speeds ... */ 2113 dev_qualifier.bMaxPacketSize0 = device_desc.bMaxPacketSize0; 2114 2115 /* and that all endpoints are dual-speed */ /drivers/usb/gadget/ether.c: 2071 in eth_bind() 2065 2066 #if defined(CONFIG_USB_ETH_CDC) || defined(CONFIG_USB_ETH_RNDIS) 2067 /* 2068 * CDC Ethernet control interface doesn't require a status endpoint. 2069 * Since some hosts expect one, try to allocate one anyway. 2070 */
CID 501791: (DEADCODE) Execution cannot reach the expression "rndis" inside thisstatement: "if (cdc || rndis) { statu...". 2071 if (cdc || rndis) { 2072 status_ep = usb_ep_autoconfig(gadget, &fs_status_desc); 2073 if (status_ep) { 2074 status_ep->driver_data = status_ep; /* claim */ 2075 } else if (rndis) { 2076 pr_err("can't run RNDIS on %s", gadget->name); /drivers/usb/gadget/ether.c: 2089 in eth_bind() 2083 } 2084 } 2085 #endif 2086 2087 /* one config: cdc, else minimal subset */ 2088 if (!cdc) {
CID 501791: (DEADCODE) Execution cannot reach this statement: "eth_config.bNumInterfaces =1;". 2089 eth_config.bNumInterfaces = 1; 2090 eth_config.iConfiguration = STRING_SUBSET; 2091 2092 /* 2093 * use functions to set these up, in case we're built to work 2094 * with multiple controllers and must override CDC Ethernet.
** CID 501790: Null pointer dereferences (FORWARD_NULL) /cmd/bcb.c: 175 in __bcb_initialize()
*** CID 501790: Null pointer dereferences (FORWARD_NULL) /cmd/bcb.c: 175 in __bcb_initialize() 169 } 170 } 171 172 return CMD_RET_SUCCESS; 173 174 err_read_fail:
CID 501790: Null pointer dereferences (FORWARD_NULL) Dereferencing null pointer "block".175 printf("Error: %d %d:%s read failed (%d)\n", block->uclass_id, 176 block->devnum, partition->name, ret); 177 __bcb_reset(); 178 return CMD_RET_FAILURE; 179 } 180
This probably deserves to be addressed. I don't know if Dmitrii is actively watching the list so I'll study this in more detail and send a fix if appropriate.
Fix submitted here:
https://lore.kernel.org/all/20240724-bcb-crash-v1-1-44caff15bce4@baylibre.co...
** CID 501789: Insecure data handling (TAINTED_SCALAR) /lib/tpm_tcg2.c: 41 in tcg2_get_pcr_info()
*** CID 501789: Insecure data handling (TAINTED_SCALAR) /lib/tpm_tcg2.c: 41 in tcg2_get_pcr_info() 35 memset(response, 0, sizeof(response)); 36 37 ret = tpm2_get_pcr_info(dev, &pcrs); 38 if (ret) 39 return ret; 40
CID 501789: Insecure data handling (TAINTED_SCALAR) Using tainted variable "pcrs.count" as a loop boundary.41 for (i = 0; i < pcrs.count; i++) { 42 u32 hash_mask = tcg2_algorithm_to_mask(pcrs.selection[i].hash); 43 44 if (hash_mask) { 45 *supported_pcr |= hash_mask; 46 if (tpm2_is_active_pcr(&pcrs.selection[i]))
** CID 501788: Memory - corruptions (OVERRUN)
*** CID 501788: Memory - corruptions (OVERRUN) /lib/tpm_tcg2.c: 658 in tcg2_measurement_term() 652 bool error) 653 { 654 u32 event = error ? 0x1 : 0xffffffff; 655 int i; 656 657 for (i = 0; i < 8; ++i)
CID 501788: Memory - corruptions (OVERRUN) Overrunning buffer pointed to by "(u8 const *)&event" of 4 bytes bypassing it to a function which accesses it at byte offset 63. 658 tcg2_measure_event(dev, elog, i, EV_SEPARATOR, sizeof(event), 659 (const u8 *)&event); 660 661 if (elog->log) 662 unmap_physmem(elog->log, MAP_NOCACHE); 663 }
----- End forwarded message -----
-- Tom