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

Tom Rini
2020-05-26T20:10:13Z

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.

Option C is to just mark this (and the similar ones you can see via the
dashboard) as false positive.

--
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/d26215cb/attachment.sig>