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

[Tom Rini <trini@konsulko.com>]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200526/d26215cb/attachment.sig>