[PATCH 1/1] dtc: fix asm_emit_data()

[PATCH 1/1] dtc: fix asm_emit_data()

[Heinrich Schuchardt]

((d.len - off) >= sizeof(uint32_t)) will possibly be true if off > 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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 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) {
 		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;
 	}
-- 
2.40.1

Re: [PATCH 1/1] dtc: fix asm_emit_data()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]