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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

On mer., juil. 24, 2024 at 11:21, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

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

Fix submitted here:

https://lore.kernel.org/all/20240724-bcb-crash-v1-1-44caff15bce4@baylibre.com/

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