Hi Tom,
Thank you for the report.
On mar., juil. 23, 2024 at 08:18, Tom Rini <tr...@konsulko.com> wrote:
> Here's the latest report.
>
> ---------- Forwarded message ---------
> From: <scan-ad...@coverity.com>
> Date: Mon, Jul 22, 2024, 8:07 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To: <tom.r...@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 an
> offset.
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 a
> function 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: "return
> 9223372036854775817UL;".
> 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 this
> statement: "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 this
> statement: "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.
>
> ** 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 by
> passing 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
