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.