dt: fix integer check in dt_device_is_compatible

cplen is unsigned, thus, it can never be < 0. Use a signed integer local variable instead.

Signed-off-by Stefano Stabellini <[email protected]>

---
  xen/common/device_tree.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

xen/common/device_tree.c +5 -2

index 8fc401d..df274cc 100644
@@ -213,17 +213,20 @@ bool_t dt_device_is_compatible(const struct dt_device_node *device,
  {
      const char* cp;
      u32 cplen, l;
+    s64 ilen;
 
      cp = dt_get_property(device, "compatible", &cplen);
      if ( cp == NULL )
          return 0;
-    while ( cplen > 0 )
+
+    ilen = cplen;
+    while ( ilen > 0 )
      {
          if ( dt_compat_cmp(cp, compat) == 0 )
              return 1;
          l = strlen(cp) + 1;
          cp += l;
-        cplen -= l;
+        ilen -= l;
      }
 
      return 0;

Hi Stefano,

Quoted 2 lines · Stefano Stabellini

cplen is unsigned, thus, it can never be < 0. Use a signed integer local variable instead.

The current code checks > 0. Looking at the code I don't think it can ever be negative. So can you details what is the problem you are trying to resolve?

Quoted 24 lines

Signed-off-by Stefano Stabellini <[email protected]>

---
  xen/common/device_tree.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

xen/common/device_tree.c +4 -1

index 8fc401d..df274cc 100644
@@ -213,17 +213,20 @@ bool_t dt_device_is_compatible(const struct
 dt_device_node *device,
  {
      const char* cp;
      u32 cplen, l;
+    s64 ilen;
        cp = dt_get_property(device, "compatible", &cplen);
      if ( cp == NULL )
          return 0;
-    while ( cplen > 0 )
+
+    ilen = cplen;
+    while ( ilen > 0 )
      {
          if ( dt_compat_cmp(cp, compat) == 0 )

Quoted 8 lines

xen/common/device_tree.c +1 -1

              return 1;
          l = strlen(cp) + 1;
          cp += l;
-        cplen -= l;
+        ilen -= l;
      }
        return 0;
 

Signature · 1 line

Julien Grall

Quoted 8 lines · Julien Grall

Hi Stefano,

Quoted 2 lines · Stefano Stabellini

cplen is unsigned, thus, it can never be < 0. Use a signed integer local variable instead.

The current code checks > 0. Looking at the code I don't think it can ever be negative. So can you details what is the problem you are trying to resolve?

Yes, it can be "negative" (not actually negative because it is defined as unsigned), in fact it happens with the nodes added dynamically by grub at boot. This patches fixes booting from grub.

Specifically ilen is initialed to 16, but strlen+1 is 17, so length becomes -1. The length of the property generated by grub seems to be short by 1.

Quoted 37 lines

Quoted 33 lines

Signed-off-by Stefano Stabellini <[email protected]>

---
   xen/common/device_tree.c | 7 +++++--
   1 file changed, 5 insertions(+), 2 deletions(-)

xen/common/device_tree.c +5 -2

index 8fc401d..df274cc 100644
@@ -213,17 +213,20 @@ bool_t dt_device_is_compatible(const struct
 dt_device_node *device,
   {
       const char* cp;
       u32 cplen, l;
+    s64 ilen;
         cp = dt_get_property(device, "compatible", &cplen);
       if ( cp == NULL )
           return 0;
-    while ( cplen > 0 )
+
+    ilen = cplen;
+    while ( ilen > 0 )
       {
           if ( dt_compat_cmp(cp, compat) == 0 )
               return 1;
           l = strlen(cp) + 1;
           cp += l;
-        cplen -= l;
+        ilen -= l;
       }
         return 0;
 

Signature · 1 line

Julien Grall

Hi Stefano,

Quoted 12 lines · Stefano Stabellini

Quoted 10 lines · Julien Grall

Hi Stefano,

Quoted 2 lines · Stefano Stabellini

cplen is unsigned, thus, it can never be < 0. Use a signed integer local variable instead.

The current code checks > 0. Looking at the code I don't think it can ever be
negative. So can you details what is the problem you are trying to resolve?

Quoted 7 lines

Yes, it can be "negative" (not actually negative because it is defined as unsigned), in fact it happens with the nodes added dynamically by grub at boot. This patches fixes booting from grub.

Specifically ilen is initialed to 16, but strlen+1 is 17, so length becomes -1. The length of the property generated by grub seems to be short by 1.

Such explanation should have been in the commit message, it helps to diagnostics where the problem is coming from.

Looking at the specification [1] section 2.3.1, a compatible property is a concatenated list of null terminated strings. So the current code is actually correct and there is a bug in GRUB.

This bug was actually fixed by commit ae5817f1d "arm64/xen_boot: Fix Xen boot using GRUB2 on AARCH64".

I assume you are using Grub 2.02 which does not contain the full support for Xen. So I would not even consider to work-around it in Xen.

Instead, I would recommend you to upgrade to a more recent GRUB. It would be more likely staging as there are no new GRUB version.

Another solution is to use chainloading.