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



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.

Please, use log_err() for writing error messages. We don't need "Error:"
at the beginning of error messages.

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

Reply via email to