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-ad...@coverity.com> > Date: Mon, Nov 6, 2023 at 2:58 PM > Subject: New Defects reported by Coverity Scan for Das U-Boot > To: <tom.r...@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-9973d10a4...@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-tr...@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() > > [..]