Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
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.
** 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.
Best regards
Heinrich
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.
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.
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.
participants (2)
-
Heinrich Schuchardt -
Tom Rini