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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3364 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]