head -r356640 and mixed signed/unsigned comparisons

[Justin Hibbits]

On Sat, 11 Jan 2020 17:28:22 -0800
Mark Millard <marklmi at yahoo.com> wrote:

> The code:
> 
> 394	                for (j = 0; j < addr_cells - 1; j++) {
> 
> looks wrong to me. Evidence follows.
> 
> /usr/src/sys/dev/ofw/openfirm.h:typedef uint32_t        pcell_t;
> /usr/include/sys/_stdint.h:typedef      __uint32_t
> uint32_t; /usr/include/machine/_types.h:typedef   unsigned int
>     __uint32_t;
> 
> So pcell_t and int are of the same rank but different
> unsigned vs. signed status.
> 
> From sys/powerpc/mpc85xx/lbc.c :
> 
> 363	static int
> 364	fdt_lbc_reg_decode(phandle_t node, struct lbc_softc *sc,
> 365	    struct lbc_devinfo *di)
> 366	{
> . . .
> 369	        pcell_t addr_cells, size_cells;
> . . .
> 371	        int i, j, rv, bank;
> . . .
> 394	                for (j = 0; j < addr_cells - 1; j++) {
> 395	                        start <<= 32;
> 396	                        start |= reg[j];
> 397	                }
> 
> So, for line 394, after the usual arithmetic
> conversions for the "-" and then for the "<"
> the line would look like (consolidating
> some relevant material to be more
> textually-local for ease of comparison):
> 
> for (int j = 0; (unsigned int)j < addr_cells-1u; j++)
> 
> For addr_cells==0u: addr_cells-1u == UINT_MAX .
> 
> So, for addr_cells==0u substituted (at run-time):
> 
> for (int j = 0; (unsigned int)j < UINT_MAX; j++)
> 
> So, unless it is guaranteed that 0u<addr_cells,
> there would appear to be a looping problem here.
> 
> It appears to me that the condition:
> 
> addr_cells==0u && 0u<size_cells
> 
> would fail the tuples <= 0 test and so continue
> on to try to use line 394.
> 
> But I do not have context to know the
> condition would be a worrisome issue.

The only way addr_cells and size_cells can be zero is if the device
tree explicitly has #address-cells = <0> and #size-cells = <0>.  It's
not impossible, but it's not reasonable, and violates the requirements
of the node.

It doesn't hurt to add checks, but it's not a priority either.

- Justin