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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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?

Best regards

Heinrich


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