[tom.rini at gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Johan Jonker
jbx6244 at gmail.com
Wed Nov 8 00:18:05 CET 2023
Hi Tom, Simon,
Please have a look some comments below at 3 issues that are introduced by meself. ;)
On 11/6/23 21:27, Tom Rini wrote:
> Hey all,
>
> Here's the latest report. I _think_ I passed the right options to
> get_maintainer.pl such that it would only look far enough back in git to
> find the likely authors (along with listed maintainers of the files).
>
> ---------- Forwarded message ---------
> From: <scan-admin at coverity.com>
> Date: Mon, Nov 6, 2023 at 2:58 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To: <tom.rini at gmail.com>
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
>
> 13 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> 5 defect(s), reported by Coverity Scan earlier, were marked fixed in
> the recent build analyzed by Coverity Scan.
>
> New defect(s) Reported-by: Coverity Scan
> Showing 13 of 13 defect(s)
>
>
> ** CID 467411: Memory - corruptions (OVERRUN)
>
>
> ________________________________________________________________________________________________________
[..]
> *** CID 467407: Uninitialized variables (UNINIT)
> /drivers/scsi/scsi.c: 612 in do_scsi_scan_one()
> 606
> 607 bdesc = dev_get_uclass_plat(bdev);
> 608 bdesc->target = id;
> 609 bdesc->lun = lun;
> 610 bdesc->removable = bd.removable;
> 611 bdesc->type = bd.type;
>>>> CID 467407: Uninitialized variables (UNINIT)
>>>> Using uninitialized value "bd.bb".
> 612 bdesc->bb = bd.bb;
> 613 memcpy(&bdesc->vendor, &bd.vendor, sizeof(bd.vendor));
> 614 memcpy(&bdesc->product, &bd.product, sizeof(bd.product));
> 615 memcpy(&bdesc->revision, &bd.revision, sizeof(bd.revision));
> 616 if (IS_ENABLED(CONFIG_SYS_BIG_ENDIAN)) {
> 617 ata_swap_buf_le16((u16 *)&bdesc->vendor,
> sizeof(bd.vendor) / 2);
>
> ** CID 467406: Memory - corruptions (OVERRUN)
>
>
Scsi devices are not my thing.
I'm forced to poke in drivers, because someone else is plumbing bounce buffer code to our block class.
Introduced by:
[PATCH v5 4/8] rockchip: block: blk-uclass: add bounce buffer flag to blk_desc
https://lore.kernel.org/u-boot/ee332375-8812-8e12-da1c-9973d10a41af@gmail.com/
static void scsi_init_dev_desc_priv(struct blk_desc *dev_desc)
{
[..]
#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
dev_desc->bb = true;
#endif /* CONFIG_BOUNCE_BUFFER */
}
[..]
struct blk_desc bd;
scsi_init_dev_desc_priv(&bd);
bdesc->bb = bd.bb;
===
https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html
GLOBAL_INITIALISERS
Global variables should not be initialized explicitly to 0 (or NULL, false, etc.). Your compiler (or rather your loader, which is responsible for zeroing out the relevant sections) automatically does it for you.
INITIALISED_STATIC
Static variables should not be initialized explicitly to zero. Your compiler (or rather your loader) automatically does it for you.
===
I assumed that variables are always zeroed in the blk_desc structure.
The value of bb only matters if IS_ENABLED(CONFIG_BOUNCE_BUFFER).
For scsi:
dev_desc->bb = IS_ENABLED(CONFIG_BOUNCE_BUFFER) ? true : false;
The scsi block class worked fine for years without bounce buffer. Do all scsi devices need that buffer all of a sudden? What didn't work then?
Please advise what is the best approach.
===
There is a patch in review that changes this file. Better let that go in first before changing something here. What's the status?
[PATCH] scsi: Forceably finish migration to DM_SCSI
https://lore.kernel.org/u-boot/20231028005951.1187616-1-trini@konsulko.com/
> ________________________________________________________________________________________________________
[..]
> ________________________________________________________________________________________________________
> *** CID 467402: (CHECKED_RETURN)
> /drivers/block/rkmtd.c: 737 in rkmtd_init_plat()
> 731
> 732 debug("starting_lba : %llu\n",
> le64_to_cpu(plat->gpt_e->starting_lba));
> 733 debug("ending_lba : %llu\n",
> le64_to_cpu(plat->gpt_e->ending_lba));
> 734
> 735 memcpy(plat->gpt_e->partition_type_guid.b,
> &partition_basic_data_guid, 16);
> 736
>>>> CID 467402: (CHECKED_RETURN)
>>>> Calling "uuid_str_to_bin" without checking return value (as is done elsewhere 9 out of 11 times).
> 737 uuid_str_to_bin(plat->uuid_part_str,
> plat->gpt_e->unique_partition_guid.b,
> 738 UUID_STR_FORMAT_GUID);
Comment 2:
gen_rand_uuid_str(plat->uuid_disk_str, UUID_STR_FORMAT_GUID);
uuid_str_to_bin(plat->uuid_part_str, plat->gpt_e->unique_partition_guid.b,
UUID_STR_FORMAT_GUID);
The function uuid_str_to_bin() gets a string from gen_rand_uuid_str() which is guarantied correct.
Checking the output is unnecessary.
if (!uuid_str_valid(uuid_str)) {
return -EINVAL;
}
This is more a false positive. What should we do with it?
> 739
> 740 efiname_len = sizeof(plat->gpt_e->partition_name) /
> sizeof(efi_char16_t);
> 741 dosname_len = sizeof(name);
> 742
> /drivers/block/rkmtd.c: 755 in rkmtd_init_plat()
> 749 plat->gpt_h->header_size = cpu_to_le32(sizeof(gpt_header));
> 750 plat->gpt_h->first_usable_lba = cpu_to_le64(64);
> 751 plat->gpt_h->last_usable_lba = cpu_to_le64(LBA - 34);
> 752 plat->gpt_h->num_partition_entries = cpu_to_le32(1);
> 753 plat->gpt_h->sizeof_partition_entry =
> cpu_to_le32(sizeof(gpt_entry));
> 754
>>>> CID 467402: (CHECKED_RETURN)
>>>> Calling "uuid_str_to_bin" without checking return value (as is done elsewhere 9 out of 11 times).
> 755 uuid_str_to_bin(plat->uuid_disk_str, plat->gpt_h->disk_guid.b,
> 756 UUID_STR_FORMAT_GUID);
Comment 3:
Dito as comment 2.
> 757
> 758 plat->gpt_h->partition_entry_array_crc32 = 0;
> 759 calc_crc32 = efi_crc32((const unsigned char *)plat->gpt_e,
> 760
> le32_to_cpu(plat->gpt_h->num_partition_entries) *
>
> ** CID 467401: Memory - corruptions (OVERRUN)
> /drivers/firmware/scmi/sandbox-scmi_agent.c: 629 in sandbox_scmi_pwd_state_set()
>
>
[..]
More information about the U-Boot
mailing list