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

Heinrich Schuchardt
2020-05-26T20:02:36Z

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