Am 24. Juli 2024 11:56:17 MESZ schrieb Mattijs Korpershoek
<mkorpersh...@baylibre.com>:
>Hi Heinrich,
>
>On mer., juil. 24, 2024 at 11:45, Heinrich Schuchardt <xypron.g...@gmx.de>
>wrote:
>
>> On 24.07.24 11:21, Mattijs Korpershoek wrote:
>>> 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.
>>
>> The warning is specifically about invoking free for the buffer that we
>> have allocated via malloc(). Our implementation of malloc() and free()
>> stores some meta-information about allocated buffers at a negative
>> offset and we don't overwrite this area via blk_read().
>
>Ok, so does that mean that you agree that this code is safe and we don't
>need any further action to fix it?
No fix needed.
Tom just needs to nark it in Coverity as "intended".
>
>>
>>>
>>>
>>>> 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.
>>
>> If blk_get_dev() returns NULL, we should write a message like "No such
>> device" and return CMD_RET_FAILURE immediately.
>
>Yes, thank you, I've submitted a fix:
>
>https://lore.kernel.org/all/20240724-bcb-crash-v1-1-44caff15b...@baylibre.com/
>
>
>>
>> Please, use log_err() for writing error messages. We don't need "Error:"
>> at the beginning of error messages.
>
>Is log_err() also the preferred way for commands? Since they are
>interactive, it seems odd to have an "optional" message.
>
>If it is, I'll convert the whole file in a separate, future patch.
Messages written with the log functions can be written to a syslog server.
Remote message logging may be of interest for commands executed in scripts.
Best regards
Heinrich
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>
>>>> ** 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
