On Wed, Nov 08, 2023 at 12:18:05AM +0100, Johan Jonker wrote: > Hi Tom, Simon, > > Please have a look some comments below at 3 issues that are introduced by > meself. ;)
Thanks for looking.
>
> 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.
OK. So I think Marek might be better able to comment on why yes, we need
bounce buffers more often than we used to, for sanity sake. But...
> 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.
Popping back to do_scsi_scan_one(), 'bd' isn't a global variable, it's a
function variable. And before bounce buffers, scsi_init_dev_desc_priv()
which is called a bit earlier in the function that Coverity is talking
about here would initialize every member, as you note in your analysis.
But it doesn't set bb in the case of CONFIG_BOUNCE_BUFFER=n. A simple
fix to this would be for that function to memset everything to 0 an then
change any values it needs to set. I've done that locally and will post
and CC you for sanity testing all the same.
> ===
> 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/
I've merged that to next this morning, but it doesn't change this part
of the code (it was removing legacy code).
> > ________________________________________________________________________________________________________
>
> [..]
>
> > ________________________________________________________________________________________________________
> > *** 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?
In Coverity scan terms "I know what I did here and it's right" is I
think "Intentional" and not "False Positive" (where that's supposed to
mean the tool is wrong and should have known better). Thanks for
explaining, I'll go mark it as such in the dashboard for both cases.
--
Tom
signature.asc
Description: PGP signature
