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

[Tom Rini <trini@konsulko.com>]

On Tue, May 26, 2020 at 10:36:44PM +0200, Heinrich Schuchardt wrote:
> On 26.05.20 22:10, Tom Rini wrote:
> > On Tue, May 26, 2020 at 10:02:36PM +0200, Heinrich Schuchardt
> > wrote:
> >> On 26.05.20 20:40, Tom Rini wrote:
> >>> Ah, I thought you might not have been part of Coverity.
> >>> https://scan.coverity.com/projects/das-u-boot is where to
> >>> start, it will take GitHub credentials and then I can approve
> >>> you.
> >>
> >> Thanks for granting access. In the GUI one can really drill down
> >> into the explanation of the problem. This is very helpful.
> >
> > And thanks for digging more!
> >
> >>
> >>> ** CID 303760:    (TAINTED_SCALAR)
> >>>
> >>>
> >>>
> >> ________________________________________________________________________________________________________
> >>>
> >>
> *** CID 303760:    (TAINTED_SCALAR)
> >>> /cmd/efidebug.c: 938 in show_efi_boot_order() 932
> >>> } 933                     p = label; 934
> >>> utf16_utf8_strncpy(&p, lo.label, label_len16); 935
> >>> printf("%2d: %s: %s\n", i + 1, var_name, label); 936
> >>> free(label); 937
> >>>>>> CID 303760:    (TAINTED_SCALAR) Passing tainted variable
> >>>>>> "data" to a tainted sink.
> >>> 938                     free(data); 939             } 940
> >>> out: 941             free(bootorder); 942 943
> >>> return ret; /cmd/efidebug.c: 929 in show_efi_boot_order() 923
> >>> efi_deserialize_load_option(&lo, data); 924 925
> >>> label_len16 = u16_strlen(lo.label); 926
> >>> label_len = utf16_utf8_strnlen(lo.label,
> >> label_len16);
> >>> 927                     label = malloc(label_len + 1); 928
> >>> if (!label) {
> >>>>>> CID 303760:    (TAINTED_SCALAR) Passing tainted variable
> >>>>>> "data" to a tainted sink.
> >>> 929                             free(data); 930
> >>> ret = CMD_RET_FAILURE; 931                             goto
> >>> out; 932                     } 933                     p =
> >>> label; 934                     utf16_utf8_strncpy(&p, lo.label,
> >>> label_len16);
> >>
> >> In CID 303760 the logic is as follows:
> >>
> >> In show_efi_boot_order() we malloc() memory. The allocated buffer
> >> is filled via byte swapping (get_unaligned_le16(),
> >> get_unaligned_le32()).
> >>
> >> Here comes Coverity's logic: "byte_swapping: Performing a byte
> >> swapping operation on ... implies that it came from an external
> >> source, and is therefore tainted."
> >>
> >> Now we pass the pointer to free(). Free() looks at 16 bytes
> >> preceding the pointer. Therefore free() is considered a tainted
> >> sink and an issue is raised.
> >>
> >> https://community.synopsys.com/s/article/From-Case-Clearing-TAINTED-STRING
> >>
> >>
> suggests to use Coverity specific comments to mark cleansing functions.
> >> This is not what I am inclined to do.
> >>
> >> CCing Takahiro as he had a hand in this code.
> >
> > So, option B on that link is what I was thinking about which is
> > creating a function in the model file to tell Coverity it's
> > handled.  I was going to include what ours was already as I thought
> > I had written one, but there's not one in the dashboard currently.
> > And frankly a drawback of Coverity is that you can't iterate on
> > testing those kind of things easily.
> 
> Here are example model files for Coverity:
> 
> https://github.com/qemu/qemu/blob/master/scripts/coverity-model.c
> https://github.com/python/cpython/blob/master/Misc/coverity_model.c
> 
> How many functions do you think we will have to maintain in the model
> file? Who will take the effort?

Ah yes, I think I looked at those a while ago and didn't come up with
anything that reduced our defects so I set it aside to look harder at
later.  And haven't yet cycled back.

I would say once we have an initial functional skeleton in, any time
someone sees a Coverity defect that's a false positive and wants to
write a model function to cover it rather than just close it out in the
dashboard, we'll get an update to it and I'll push it to Coverity.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200526/79606bb2/attachment.sig>