On Mon, Jul 14 2025, Tom Rini <tr...@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-ad...@coverity.com>
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To: <tom.r...@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
