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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Heinrich,

On mer., juil. 24, 2024 at 11:45, Heinrich Schuchardt <xypron.glpk@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@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.
>
> 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-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.

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