Fwd: New Defects reported by Coverity Scan for Das U-Boot

Heinrich Schuchardt xypron.glpk at gmx.de
Wed Jul 24 12:06:46 CEST 2024



Am 24. Juli 2024 11:56:17 MESZ schrieb Mattijs Korpershoek <mkorpershoek at baylibre.com>:
>Hi Heinrich,
>
>On mer., juil. 24, 2024 at 11:45, Heinrich Schuchardt <xypron.glpk at 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 <trini at konsulko.com> wrote:
>>>
>>>> Here's the latest report.
>>>>
>>>> ---------- Forwarded message ---------
>>>> From: <scan-admin at coverity.com>
>>>> Date: Mon, Jul 22, 2024, 8:07 PM
>>>> Subject: New Defects reported by Coverity Scan for Das U-Boot
>>>> To: <tom.rini at 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-44caff15bce4@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


More information about the U-Boot mailing list