On Wed, Jul 24, 2024 at 12:06:46PM +0200, Heinrich Schuchardt wrote:
>
>
> Am 24. Juli 2024 11:56:17 MESZ schrieb Mattijs Korpershoek
> <mkorpersh...@baylibre.com>:
> >Hi Heinrich,
> >
> >On mer., juil. 24, 2024 at 11:45, Heinrich Schuchardt <xypron.g...@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 <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().
> >
> >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. -- Tom
signature.asc
Description: PGP signature
