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

[Rasmus Villemoes <ravi@prevas.dk>]

On Mon, Jul 14 2025, Tom Rini <trini@konsulko.com> wrote:

> Here's the latest report from Coverity. Good news is closing 5 existing
> issues (overlap with smatch I think) but 3 new ones. Or maybe it's
> related to Rasmus' cleanup series? I can only run one report a day I
> think so I don't have granular breakdown on which changes today brought
> these up.
>
> From: <scan-admin@coverity.com>
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To: <tom.rini@gmail.com>
> Date: Mon, Jul 14, 2025 at 5:23 PM (1 day, 9 hours, 56 minutes ago)
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to *Das U-Boot*
> found with Coverity Scan.
>
>    - *New Defects Found:* 3
>    - 5 defect(s), reported by Coverity Scan earlier, were marked fixed in
>    the recent build analyzed by Coverity Scan.
>    - *Defects Shown:* Showing 3 of 3 defect(s)
>
> Defect Details
>
> ** CID 573150:       Integer handling issues  (INTEGER_OVERFLOW)
> /drivers/pci/pci-uclass.c: 1531           in dm_pci_map_ea_virt()
>
>
> _____________________________________________________________________________________________
> *** CID 573150:         Integer handling issues  (INTEGER_OVERFLOW)
> /drivers/pci/pci-uclass.c: 1531             in dm_pci_map_ea_virt()
> 1525     		if (ea_entry & PCI_EA_IS_64) {
> 1526     			/* MaxOffset 2nd DW */
> 1527     			dm_pci_read_config32(dev, ea_off + 16, &ea_entry);
> 1528     			sz |= ((u64)ea_entry) << 32;
> 1529     		}
> 1530
>>>>     CID 573150:         Integer handling issues  (INTEGER_OVERFLOW)
>>>>     Expression "sz + 1UL", where "sz" is known to be equal to 18446744073709551615, overflows the type of "sz + 1UL", which is type "unsigned long".
> 1531     		addr = (pdata->virtid - 1) * (sz + 1);
> 1532     	}
> 1533

I don't see how this one could be due to the int limit patches, as I see
no reference to any _MIN/_MAX macro, also not indirectly via the
definition of PCI_EA_FIELD_MASK.

I also have no idea how Coverity can think that sz can be known to be
equal to ~0ULL. Sure, if it phrased it "if sz is equal to ..., then sz+1
overflows", but that's not what it says. Nor would that be very useful,
as just about _any_ arithmetic expression can overflow for _some_ values
of the referenced variables.

Honestly, this sounds like it has been AI-infected.

> 1534     	return addr;
> 1535     }
> 1536
>
> ** CID 573149:       Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> /lib/efi_loader/efi_file.c: 594           in efi_file_read_int()
>
>
> _____________________________________________________________________________________________
> *** CID 573149:         Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> /lib/efi_loader/efi_file.c: 594             in efi_file_read_int()
> 588
> 589     	bs = *buffer_size;
> 590     	if (fh->isdir)
> 591     		ret = dir_read(fh, &bs, buffer);
> 592     	else
> 593     		ret = file_read(fh, &bs, buffer);
>>>>     CID 573149:         Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
>>>>     "bs <= 18446744073709551615ULL /* 9223372036854775807LL * 2ULL + 1ULL */" is always true regardless of the values of its operands. This occurs as the logical operand of "if".
> 594     	if (bs <= SIZE_MAX)
> 595     		*buffer_size = bs;
> 596     	else
> 597     		*buffer_size = SIZE_MAX;
> 598
> 599     	return ret;
>

So this one might be triggered by the new definition of SIZE_MAX, though
SIZE_MAX was also a compile-time (though not cpp) constant previously. I
think we should define SIZE_MAX properly instead of via that UINTPTR_MAX
indirection, which itself could use some cleanup.

But aside from that, we should be able to silence Coverity by either
just changing the <= to < (because in the == case the other branch of
the if would have the same effect, but it's no longer a tautology). Or
we could maybe do *buffer_size = min_t(u64, bs, SIZE_MAX), though that
might expand to something with the exact same problem.


> ** CID 573148:       Integer handling issues  (INTEGER_OVERFLOW)
> /drivers/pci/pci-uclass.c: 1581           in dm_pci_map_ea_bar()
>
>
> _____________________________________________________________________________________________
> *** CID 573148:         Integer handling issues  (INTEGER_OVERFLOW)
> /drivers/pci/pci-uclass.c: 1581             in dm_pci_map_ea_bar()
> 1575     			addr |= ((u64)ea_entry) << 32;
> 1576     		}
> 1577
> 1578     		if (IS_ENABLED(CONFIG_PCI_SRIOV))
> 1579     			addr += dm_pci_map_ea_virt(dev, ea_off, pdata);
> 1580
>>>>     CID 573148:         Integer handling issues  (INTEGER_OVERFLOW)
>>>>     Expression "4294967295U - addr", where "addr" is known to be equal to 4294967292, underflows the type of "4294967295U - addr", which is type "unsigned int".
> 1581     		if (~((phys_addr_t)0) - addr < offset)
> 1582     			return NULL;
> 1583

Wait, what? Just to be completely sure, I copy-pasted those two numbers:

4294967295
4294967292

I think my 8-year old can see that subtracting the second from the first
does not lead to a negative result.

So from my chair, that's another point added to the AI hypothesis.

Rasmus