From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 26 May 2020 16:48:19 -0400 Subject: Fwd: New Defects reported by Coverity Scan for Das U-Boot In-Reply-To: <6d5b6b45-c0cc-9b0e-e2b3-ca959e548102@gmx.de> References: <5ecd3c8249d1_d6f562acb748daf5820386@appnode-2.mail> <8ea1ca2f-2826-58f2-4b6b-ed5cfe977467@gmx.de> <20200526184027.GJ12717@bill-the-cat> <20200526201013.GN12717@bill-the-cat> <6d5b6b45-c0cc-9b0e-e2b3-ca959e548102@gmx.de> Message-ID: <20200526204819.GO12717@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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: