Fwd: New Defects reported by Coverity Scan for Das U-Boot
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. 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
** 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 -----
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 anoffset.
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 afunction 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: "return9223372036854775817UL;". 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 thisstatement: "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 thisstatement: "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 bypassing 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
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 anoffset.
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 afunction 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: "return9223372036854775817UL;". 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 thisstatement: "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 thisstatement: "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 bypassing 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
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 anoffset.
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 afunction 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: "return9223372036854775817UL;". 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 thisstatement: "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 thisstatement: "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.co...
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 bypassing 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
Am 24. Juli 2024 11:56:17 MESZ schrieb 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?
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.co...
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
On Wed, Jul 24, 2024 at 12:06:46PM +0200, Heinrich Schuchardt wrote:
Am 24. Juli 2024 11:56:17 MESZ schrieb 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?
No fix needed.
Tom just needs to nark it in Coverity as "intended".
Thanks. I'll copy/paste the explanation in and close it next time I'm over there.
On mer., juil. 24, 2024 at 16:40, Tom Rini trini@konsulko.com wrote:
[...]
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".
Thanks. I'll copy/paste the explanation in and close it next time I'm over there.
I've done it already, added a lore link to this thread as justification.
-- Tom
On Thu, Jul 25, 2024 at 10:04:33AM +0200, Mattijs Korpershoek wrote:
On mer., juil. 24, 2024 at 16:40, Tom Rini trini@konsulko.com wrote:
[...]
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".
Thanks. I'll copy/paste the explanation in and close it next time I'm over there.
I've done it already, added a lore link to this thread as justification.
Thanks!
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 anoffset.
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 afunction 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: "return9223372036854775817UL;". 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 thisstatement: "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 thisstatement: "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.co...
** 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 bypassing 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
participants (3)
-
Heinrich Schuchardt -
Mattijs Korpershoek -
Tom Rini