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
signature.asc
Description: PGP signature
