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. >
