[PATCH for-4.12] dt: fix integer check in dt_device_is_compatible

[PATCH for-4.12] dt: fix integer check in dt_device_is_compatible

[Stefano Stabellini]

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

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/common/device_tree.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 8fc401d..df274cc 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -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;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] dt: fix integer check in dt_device_is_compatible

[Julien Grall]

Hi Stefano,

On 11/12/2018 18:46, Stefano Stabellini wrote:
> 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?

Cheers,

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/common/device_tree.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 8fc401d..df274cc 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -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;
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] dt: fix integer check in dt_device_is_compatible

[Stefano Stabellini]

On Wed, 12 Dec 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/12/2018 18:46, Stefano Stabellini wrote:
> > 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.


> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >   xen/common/device_tree.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 8fc401d..df274cc 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -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;
> > 
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] dt: fix integer check in dt_device_is_compatible

[Julien Grall]

Hi Stefano,

On 12/12/18 11:53 PM, Stefano Stabellini wrote:
> On Wed, 12 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/12/2018 18:46, Stefano Stabellini wrote:
>>> 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.
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.

Cheers,

[1] 
https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2


> 
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>>    xen/common/device_tree.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index 8fc401d..df274cc 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -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;
>>>
>>
>> -- 
>> Julien Grall
>>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] dt: fix integer check in dt_device_is_compatible

[Stefano Stabellini]

On Thu, 13 Dec 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 12/12/18 11:53 PM, Stefano Stabellini wrote:
> > On Wed, 12 Dec 2018, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 11/12/2018 18:46, Stefano Stabellini wrote:
> > > > 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.
> 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.

it is true that it was a "temporary" bug in grub

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel