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