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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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