On Tue, Aug 22, 2023 at 01:06:27PM +0200, Heinrich Schuchardt wrote:
> ((d.len - off) >= sizeof(uint32_t)) will possibly be true if off > d.len:
Technically true, but the code is constructed such that off is
always less than or equal to d.len.
> sizeof(uint32_t)) is an unsigned type. The C language will convert
> (d.len - off) to an unsigned type before executing the comparison.
>
> Correct the inequality.
> Change the equality for byte output to match the one for uint32_t.
>
> Fixes: 53359016caf6 ("dtc: Use stdint.h types throughout dtc")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@xxxxxxxxxxxxx>
> ---
> flattree.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/flattree.c b/flattree.c
> index 1bcd808..9f67104 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -156,12 +156,12 @@ static void asm_emit_data(void *e, struct data d)
> for_each_marker_of_type(m, LABEL)
> emit_offset_label(f, m->ref, m->offset);
>
> - while ((d.len - off) >= sizeof(uint32_t)) {
> + while (d.len + sizeof(uint32_t) >= off) {
This eliminates the possibility of underflow, but replaces it with a
possibility of overflow. Now there will be an overflow if d.len is
within 4 of UINT_MAX, again giving a surprising result.
Now, in practice I don't think d.len can ever exceed 2^31, but
confirming that requires looking through the entire codebase. In
contrast confirming that off <= d.len requires only examining the few
lines of this function.
So, nack.
> asm_emit_cell(e, dtb_ld32(d.val + off));
> off += sizeof(uint32_t);
> }
>
> - while ((d.len - off) >= 1) {
> + while (d.len + 1 >= off) {
> fprintf(f, "\t.byte\t0x%hhx\n", d.val[off]);
> off += 1;
> }
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
